Add support for cancelling regular hub invocations from client - #64098
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
…fying comment Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
BrennanConroy
left a comment
BrennanConroy
left a comment
There was a problem hiding this comment.
@copilot Update the comment on HubConnectionContext.ActiveRequestCancellationSources as well
…comment Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
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:
|
Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for canceling regular (non-streaming) SignalR hub invocations from the client by flowing cancellation via CancelInvocationMessage and enabling CancellationToken as a synthetic hub method argument beyond streaming scenarios.
Changes:
- Client: send
CancelInvocationMessagewhen anInvokeAsynccancellation token is canceled; refactor shared cancellation logic for invoke/stream. - Server: allow
CancellationTokensynthetic arguments for all hub methods and register per-invocation cancellation sources soCancelInvocationMessagecan cancel regular invocations. - Tests/logging: add coverage for cancellation messaging and hub method
CancellationTokenacceptance; introduce new dispatcher log events/messages.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs | Updates server-side test to validate CancellationToken works for non-streaming invocations and can be triggered via CancelInvocationMessage. |
| src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs | Adds a cancelable non-streaming hub method used by tests. |
| src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs | Treats CancellationToken parameters as synthetic for all hub methods (not just streams). |
| src/SignalR/server/Core/src/Internal/DefaultHubDispatcherLog.cs | Adds new log events/messages for canceling invocations/streams by ID. |
| src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs | Registers invocation CTS in ActiveRequestCancellationSources for non-streaming invocations; updates cancel message handling and logging. |
| src/SignalR/server/Core/src/HubConnectionContext.cs | Updates comment describing ActiveRequestCancellationSources usage. |
| src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.cs | Adds a unit test asserting CancelInvocationMessage is sent when canceling an InvokeAsync token; adjusts upload-stream cancellation expectations. |
| src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs | Implements/refactors client cancellation message sending; adjusts streaming-upload helper to return stream tasks for ordering. |
Comments suppressed due to low confidence (1)
src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs:68
- Now that CancellationToken parameters are treated as synthetic for all hub methods, methods with multiple CancellationToken parameters will result in multiple CTS instances being created in ReplaceArguments, but only the last one is returned/registered for cancellation. This leads to leaks and inconsistent cancellation behavior. Either validate that a hub method can only have a single CancellationToken parameter, or create a single CTS and reuse its Token for all CancellationToken parameters.
// Take out synthetic arguments that will be provided by the server, this list will be given to the protocol parsers
ParameterTypes = methodExecutor.MethodParameters.Where((p, index) =>
{
// CancellationTokens are synthetic arguments provided by the server
if (p.ParameterType == typeof(CancellationToken))
{
HasSyntheticArguments = true;
return false;
}
| if (cancellationToken.CanBeCanceled) | ||
| { | ||
| cancellationToken.Register(state => | ||
| { | ||
| var (irqLocal, tasks) = ((InvocationRequest, Task[]?))state!; | ||
| _ = CancelInvocationAsync(irqLocal, tasks); | ||
| }, (irq, streamTasks)); | ||
| } |
There was a problem hiding this comment.
The CancellationToken.Register callback is not being disposed. If the caller supplies a long-lived token (e.g., RequestAborted), this registration will keep the InvocationRequest (and transitively the HubConnection) alive after the invocation completes, and can also send a CancelInvocationMessage after completion if the token is canceled later. Capture the CancellationTokenRegistration and dispose it when the invocation finishes (or move this registration into InvocationRequest so it’s disposed as part of irq.Dispose).
| cancellationToken.Register(state => | ||
| { | ||
| var (irqLocal, tasks) = ((InvocationRequest, Task[]?))state!; | ||
| _ = CancelInvocationAsync(irqLocal, tasks); | ||
| }, (irq, streamTasks)); |
There was a problem hiding this comment.
Same issue as InvokeCoreAsyncCore: this CancellationToken.Register is never disposed, which can retain the InvocationRequest and related state after the stream completes and can trigger CancelInvocationMessage after completion if the token is canceled later. Store the CancellationTokenRegistration and dispose it when the stream invocation is completed/cleaned up (ideally via InvocationRequest disposal).
| cancellationToken.Register(state => | |
| { | |
| var (irqLocal, tasks) = ((InvocationRequest, Task[]?))state!; | |
| _ = CancelInvocationAsync(irqLocal, tasks); | |
| }, (irq, streamTasks)); | |
| var cancellationTokenRegistration = cancellationToken.Register(state => | |
| { | |
| var (irqLocal, tasks) = ((InvocationRequest, Task[]?))state!; | |
| _ = CancelInvocationAsync(irqLocal, tasks); | |
| }, (irq, streamTasks)); | |
| _ = channel.Completion.ContinueWith( | |
| static (t, state) => | |
| { | |
| var registration = (CancellationTokenRegistration)state!; | |
| registration.Dispose(); | |
| }, | |
| cancellationTokenRegistration, | |
| CancellationToken.None, | |
| TaskContinuationOptions.ExecuteSynchronously, | |
| TaskScheduler.Default); |
| // Register the CancellationTokenSource if present so CancelInvocationMessage can cancel it | ||
| if (cts != null && !string.IsNullOrEmpty(hubMethodInvocationMessage.InvocationId)) | ||
| { | ||
| if (!connection.ActiveRequestCancellationSources.TryAdd(hubMethodInvocationMessage.InvocationId!, cts)) | ||
| { | ||
| Log.InvocationIdInUse(logger, hubMethodInvocationMessage.InvocationId); | ||
| await SendInvocationError(hubMethodInvocationMessage.InvocationId, connection, | ||
| $"Invocation ID '{hubMethodInvocationMessage.InvocationId}' is already in use."); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
CancellationTokenSource instances created for synthetic CancellationToken parameters are now added to ActiveRequestCancellationSources but never disposed for non-streaming invocations. This will leak registrations/resources; also, if TryAdd fails due to duplicate InvocationId, the CTS should be disposed before returning. Dispose the CTS when the invocation finishes (and on the TryAdd failure path), not just remove it from the dictionary.
| case CancelInvocationMessage cancelInvocationMessage: | ||
| // Check if there is an associated active stream and cancel it if it exists. | ||
| // The cts will be removed when the streaming method completes executing | ||
| // Check if there is an associated active invocation or stream and cancel it if it exists. | ||
| // The cts will be removed when the hub method completes executing | ||
| if (connection.ActiveRequestCancellationSources.TryGetValue(cancelInvocationMessage.InvocationId!, out var cts)) | ||
| { | ||
| Log.CancelStream(_logger, cancelInvocationMessage.InvocationId!); | ||
| Log.CancelInvocation(_logger, cancelInvocationMessage.InvocationId!); | ||
| cts.Cancel(); | ||
| } | ||
| else | ||
| { | ||
| // Stream can be canceled on the server while client is canceling stream. | ||
| Log.UnexpectedCancel(_logger); | ||
| // Invocation can be canceled on the server while client is canceling invocation. | ||
| Log.UnexpectedCancelWithId(_logger, cancelInvocationMessage.InvocationId!); | ||
| } |
There was a problem hiding this comment.
DispatchMessageAsync now logs CancelInvocation/UnexpectedCancelWithId (EventIds 29/30) for CancelInvocationMessage, which changes the existing EventIds used for stream cancellation (10/11). This is a potentially breaking operational change for log/event-id based monitoring, and it also makes the “backward compatibility” methods unused. Consider keeping EventIds 10/11 for the existing stream-cancel path (or reusing them with updated message) and only using new EventIds for the new invocation-cancel scenario.
| // Remove the CancellationTokenSource from active requests | ||
| if (cts != null && !string.IsNullOrEmpty(hubMethodInvocationMessage.InvocationId)) | ||
| { | ||
| connection.ActiveRequestCancellationSources.TryRemove(hubMethodInvocationMessage.InvocationId!, out _); | ||
| } |
There was a problem hiding this comment.
This TryRemove runs whenever cts != null, even if the earlier TryAdd failed due to an InvocationId collision. In that failure case, this can remove the CTS for the existing invocation/stream that already owns the ID. Track whether the CTS was successfully registered and only remove (and dispose) it when registration succeeded.
|
I'm following this issue, can I contribute or is there any other way I can aid the progress? |
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
dotnet-policy-service
Bot
added
the
pending-ci-rerun
|
@BrennanConroy can I assist in any capacity to get this done? Please let me know? |
…TryRemove on collision - Client: store the new cancellationToken.Register registration on InvocationRequest so it is disposed with the irq, preventing a leak/post-completion CancelInvocationMessage for long-lived tokens (e.g. RequestAborted). - Server (DefaultHubDispatcher): track whether the synthetic-arg CTS was successfully added to ActiveRequestCancellationSources; only TryRemove and Dispose it when registration succeeded, so an InvocationId collision does not evict another invocation's CTS. Also dispose the CTS for non-streaming invocations. Apply the same guard to the streaming path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a hub method declared with a CancellationToken parameter is invoked via SendAsync (no InvocationId), the synthetic CTS is created and linked to ConnectionAborted but is never added to ActiveRequestCancellationSources. Move the cts?.Dispose() outside the ctsRegistered guard so it runs in that case as well, preventing per-call registration leaks on long-lived connections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…vocations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Feel free to review the PR and give your thoughts. Glad to see you're looking forward to this change. 😄 |
|
Looks good for me, too 👍 |
| private async Task CancelInvocationAsync(InvocationRequest irq, Task[]? streamTasks = null) | ||
| { | ||
| // Wait for stream completions to be sent before sending CancelInvocationMessage | ||
| if (streamTasks is not null) |
There was a problem hiding this comment.
comment here to explain that streamTasks is only client side uploading and cancellationToken is linked to uploadStreamToken at LaunchStreams?
|
/ba-g unrelated quarantine failure |
Add support for cancelling regular hub invocations from client
Description
This PR implements cancellation support for regular (non-streaming) hub method invocations from the client. Previously, only streaming invocations could be cancelled by the client. Now, when a client passes a
CancellationTokentoInvokeAsyncand cancels it, aCancelInvocationMessageis sent to the server which cancels the hub method execution via a registeredCancellationTokenSource.Client-side (
HubConnection.cs)InvokeCoreAsyncCore: registers a callback to sendCancelInvocationMessagewhen the cancellation token is triggeredCancelInvocationAsyncprivate method, used by both regular invocations and streamingServer-side (
HubMethodDescriptor.cs)CancellationTokensynthetic arguments to streaming methods only — all hub methods can now acceptCancellationTokenServer-side (
DefaultHubDispatcher.cs)ExecuteInvocationto register theCancellationTokenSourceinActiveRequestCancellationSourcesso thatCancelInvocationMessagecan cancel regular invocationsCancellationTokenSourceafter the invocation completesLogging (
DefaultHubDispatcherLog.cs)CancelInvocation(ID 29) andUnexpectedCancelWithId(ID 30) at the bottom of the file with generic messages covering both invocations and streamsCancelStream(ID 10) andUnexpectedCancel(ID 11) methods for backward compatibilityServer-side (
HubConnectionContext.cs)ActiveRequestCancellationSourcesfrom "Currently used only for streaming methods" to "Used to cancel hub invocations and streaming methods"Tests
CanCancelTokenDuringInvoke_SendsCancelInvocationto verifyCancelInvocationMessageis sent for regular invocationsCancelableInvocationhub method toLongRunningHubfor testingInvokeHubMethodCanAcceptCancellationTokenAsArgumentto verifyCancellationTokenworks for regular hub method invocationsOriginal 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
CancellationTokenfrom the client, which isn't going to work.The cancellation token in
InvokeAsyncwill 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
CancelInvocationMessagewhen the cancellation token passed in toInvokeAsyncis 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
CancelInvocationMessageaspnetcore/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
CancelInvocationMessagefrom 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.