-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add support for cancelling regular hub invocations from client #64098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: BrennanConroy <[email protected]>
Co-authored-by: BrennanConroy <[email protected]>
…fying comment Co-authored-by: BrennanConroy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Update the comment on HubConnectionContext.ActiveRequestCancellationSources as well
|
||
private async Task<object?> InvokeCoreAsyncCore(string methodName, Type returnType, object?[] args, CancellationToken cancellationToken) | ||
{ | ||
async Task OnInvocationCanceled(InvocationRequest irq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this code is the exact same as the code used for streaming. We should extract it into a private method and call that method from here and the streaming method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 0ad2858. Extracted the duplicate cancellation logic into a CancelInvocationAsync
private method that's now used by both InvokeCoreAsyncCore
and StreamAsChannelCoreAsyncCore
.
|
||
[LoggerMessage(11, LogLevel.Debug, "CancelInvocationMessage received unexpectedly.", EventName = "UnexpectedCancel")] | ||
public static partial void UnexpectedCancel(ILogger logger); | ||
[LoggerMessage(11, LogLevel.Debug, "CancelInvocationMessage received for {InvocationId} but invocation was not found.", EventName = "UnexpectedCancel")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LoggerMessage(11, LogLevel.Debug, "CancelInvocationMessage received for {InvocationId} but invocation was not found.", EventName = "UnexpectedCancel")] | |
[LoggerMessage(11, LogLevel.Debug, "CancelInvocationMessage received for {InvocationId} but invocation or stream was not found.", EventName = "UnexpectedCancel")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 0ad2858. Created a new log method UnexpectedCancelWithId
(ID 27) with the suggested message "...invocation or stream was not found." to avoid changing the existing EventName.
|
||
[LoggerMessage(10, LogLevel.Debug, "Canceling stream for invocation {InvocationId}.", EventName = "CancelStream")] | ||
public static partial void CancelStream(ILogger logger, string invocationId); | ||
[LoggerMessage(10, LogLevel.Debug, "Canceling invocation {InvocationId}.", EventName = "CancelInvocation")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change existing logs' EventName or Id. So let's make a new log for the more generic "CancelInvocation" log. Also might want to mention "Canceling invocation or stream {InvocationId}."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 0ad2858. Created a new log method CancelInvocation
(ID 26) with message "Canceling invocation or stream {InvocationId}." The old CancelStream
method (ID 10) is kept for backward compatibility.
…comment Co-authored-by: BrennanConroy <[email protected]>
Done in commit 0ad2858. Updated the comment from "Currently used only for streaming methods" to "Used to cancel hub invocations and streaming methods". Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
This PR implements cancellation of long-running hub methods from the client, as requested in issue #11542.
Changes Made (updated after review):
Client-side (HubConnection.cs):
CancelInvocationAsync
private method (used by both regular invocations and streaming)InvokeCoreAsyncCore
andStreamAsChannelCoreAsyncCore
to use the extracted methodServer-side (HubMethodDescriptor.cs):
CancellationToken
as a synthetic argument for all hub methodsServer-side (DefaultHubDispatcher.cs):
ExecuteInvocation
to register CancellationTokenSource inActiveRequestCancellationSources
CancelInvocation
for the generic caseLogging (DefaultHubDispatcherLog.cs):
CancelInvocation
with ID 26,UnexpectedCancelWithId
with ID 27) to avoid breaking existing EventName/IdServer-side (HubConnectionContext.cs):
ActiveRequestCancellationSources
from "Currently used only for streaming methods" to "Used to cancel hub invocations and streaming methods"Tests:
All tests pass except for one flaky test (
UploadStreamCancellationSendsStreamComplete
) that passes in isolation but fails when run with all tests. This appears to be a test pollution/ordering issue unrelated to the core functionality being added.Original prompt
This section details on the original issue you should resolve
<issue_title>[SignalR] Possibility to cancel long running hub method from client</issue_title>
<issue_description>I´m not quit sure if this post is more of a question or a feature request.
I noticed that the
InvokeAsync<TResult>(HubConnection, String, CancellationToken)
method accepts aCancellationToken
. As far as I understood this token only cancels the invocation (and waiting for a result, ...) on the client, while on the server side the method proceeds.Is this correct?
If so wouldn´t it be a nice feature to allow such a cancellation? I saw that Google offers this possibility in their gRPC framework aswell:
When my understanding is wrong please provide me an example on how to do a cancellation of long running hub methods from the client.</issue_description>
<agent_instructions>See comment in #11542 (comment)</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@davidfowl Today we only support cancellation for streams but there's nothing wrong with supporting this for other invocations. @BrennanConroy @anurse I don't think we do support that, but we structured the code in such a way that we could add support for it in the future. @ We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our [Triage Process](https://github.com/dotnet/aspnetcore/blob/master/docs/TriageProcess.md). @davidfowl @muzopraha What's the scenario you have with a long running hub method? @davidfowl @thorgeirk11 No updates, best to add your scenario so we have a record of what people want to use it for before we invest the time. @BrennanConroy File a new issue and show your code. @BrennanConroy CancellationToken in the hub method signature is only supported for Server to client streaming currently.This issue is tracking adding it for other cases.</comment_new>
<comment_new>@BrennanConroy
Since this feature isn't implemented yet that method signature won't work. It's basically trying to receive a serialized
CancellationToken
from the client, which isn't going to work.The cancellation token in
InvokeAsync
will cancel the client waiting for a server response, but it doesn't pass that info along to the server yet. That's what this issue is tracking adding support for.public async Task UpdateChannelAsync(TPMChannels channel, CancellationToken cts = default)
->public async Task UpdateChannelAsync(TPMChannels channel)
</comment_new><comment_new>@davidfowl
I'll defer to @BrennanConroy here.</comment_new>
<comment_new>@BrennanConroy
Basically we would need to update the client(s) to send a
CancelInvocationMessage
when the cancellation token passed in toInvokeAsync
is canceled.We currently only send that message type when the client cancels a stream
aspnetcore/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs
Line 708 in 54c0cc8
The server is mostly setup to handle
CancelInvocationMessage
aspnetcore/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs
Lines 183 to 189 in 54c0cc8
The logs would likely need to be updated since it assumes streams are the only thing that can be canceled.
We'd also need to update the logic around which hub methods can have a synthetic argument (
CancellationToken
)aspnetcore/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs
Lines 64 to 66 in 54c0cc8
A massive stretch goal would be to do the same thing for client results, which would require adding synthetic argument support on the client side and sending
CancelInvocationMessage
from the server. That work is tracked by #44831 and shouldn't restrict this issue from being worke...Fixes #11542
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.