[SignalR] Fix CTS disposal - #69158
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is small, localized, and fixes a concrete CTS disposal leak without altering the normal streaming or cleanup flow.
Review tier: Lite
Findings: None
What changed in this PR
Fixes a CancellationTokenSource lifetime leak in SignalR’s DefaultHubDispatcher streaming invocation path by ensuring the CTS is disposed when an invocation ID collision prevents it from being registered.
Changes:
- Dispose
streamCtswhenActiveRequestCancellationSources.TryAdd(invocationId, streamCts)fails due to an invocation ID already being in use. - Minor file-end formatting cleanup.
| File | Description |
|---|---|
| src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs | Ensures CTS is disposed on invocation ID collision in StreamAsync to avoid leaking linked CTS instances. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Hi @BrennanConroy. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
artl93
left a comment
There was a problem hiding this comment.
Dispose. Regression. Approved.
|
Hi @BrennanConroy. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed. |
[SignalR] Fix CTS disposal- #69158
Description
When we added cancellation support for non-streaming methods, we didn't dispose the CTS in error cases.
Customer Impact
Fixes a server reliability issue.
Regression?
Regressed in 11.0-p6
Risk
Simple change, just missed an error case
Verification
Packaging changes reviewed?