Conversation
#### AI description (iteration 1) #### PR Classification This pull request fixes backpressure cancellation issues in SignalR by introducing an extra 5-second timeout. #### PR Summary The changes enhance SignalR's message buffering by adding a timeout mechanism for backpressure situations and expanding test coverage to verify the new behavior. - `src/SignalR/common/Shared/MessageBuffer.cs`: Updated backpressure logic to use a 5-second cancellation token with TimeProvider and improved timer initialization. - `src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs`: Added new tests for default and cancellable backpressure writes and renamed an existing test for clarity. - `src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs`: Introduced and adjusted scenarios to test backpressure timeout handling in hub connection scenarios. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot -->
There was a problem hiding this comment.
Pull request overview
This PR adds a bounded timeout for SignalR MessageBuffer backpressure waits (to avoid indefinite blocking) and expands/adjusts server tests to validate the new timeout-driven cancellation behavior in both unit-level MessageBuffer tests and higher-level HubConnectionHandler reconnect scenarios.
Changes:
- Add a 5-second cancellation timeout to
MessageBuffer’s ack-based backpressure wait, usingTimeProviderwhen available. - Extend
MessageBufferTestswith new coverage for default timeout cancellation vs. caller-provided cancellation, and rename the existing pipe-backpressure cancellation test for clarity. - Extend
HubConnectionHandlerTeststo validate a “backpressure timeout” close scenario alongside existing abort/ping-timeout scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/SignalR/common/Shared/MessageBuffer.cs |
Adds a 5-second timeout to buffered backpressure waits and wires in TimeProvider for timer/CTS behavior. |
src/SignalR/server/Core/src/HubConnectionContext.cs |
Adjusts CloseMessage write cancellation behavior when using stateful reconnect/message buffering. |
src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs |
Adds tests for buffered backpressure timeout and cancellation; renames and tweaks an existing cancellation test. |
src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs |
Adds a new close scenario to validate cancellation via backpressure timeout in reconnect + buffering flows. |
Comments suppressed due to low confidence (1)
src/SignalR/server/Core/src/HubConnectionContext.cs:290
- Removing the 5-second short-lived cancellation token for CloseMessage writes means SendCloseAsync (which calls WriteAsync(closeMessage, ignoreAbort: true) with the default CancellationToken.None) can now block indefinitely under pipe backpressure when the connection is still open but the client isn't reading. The new MessageBuffer backpressure timeout only applies when the message buffer is over its limit; it doesn't bound the PipeWriter.WriteAsync/Flush path for CloseMessage in the non-buffer-full case. Consider restoring a bounded timeout for CloseMessage writes here (e.g., a 5s CTS using the hubConnectionContext TimeProvider) while still not linking to ConnectionAborted so the CloseMessage can be attempted even if abort has already been requested.
var connectionToken = hubConnectionContext.ConnectionAborted;
if (message is CloseMessage)
{
// If it's a CloseMessage, we might already have triggered the ConnectionAborted token
// We would like to successfully send the CloseMessage for graceful close which means we can't use the ConnectionAborted token.
connectionToken = CancellationToken.None;
}
// MessageBuffer can wait on things other than the PipeWriter (which is canceled by other means)
// So we need to make sure the cancellation token passed to it is also canceled when the connection is aborted
using var _ = CancellationTokenUtils.CreateLinkedToken(connectionToken, cancellationToken, out var linkedToken);
var result = await messageBuffer.WriteAsync(message, linkedToken);
| // Write will hit pipe backpressure | ||
| var writeTask = messageBuffer.WriteAsync(new SerializedHubMessage(new InvocationMessage("t", new object[] { new byte[40] })), cts.Token); | ||
| Assert.False(writeTask.IsCompleted); |
There was a problem hiding this comment.
In these new buffered-backpressure tests, the write is blocked waiting for Ack (MessageBuffer backpressure), not primarily by the PipeWriter pause/resume thresholds. The comment currently says "pipe backpressure", which is misleading for what the test is asserting. Consider updating the comment to reflect that this is MessageBuffer/ack-based backpressure.
| #if NET8_0_OR_GREATER | ||
| timeProvider ??= TimeProvider.System; | ||
| _timer = new(AckRate, timeProvider); | ||
| _timeProvider = timeProvider; |
There was a problem hiding this comment.
MessageBuffer now stores the provided TimeProvider without validating it (previously it fell back to TimeProvider.System when null). Since this shared source is compiled into multiple assemblies, a null TimeProvider passed accidentally would become a NullReferenceException later (timer creation / backpressure CTS). Consider either throwing ArgumentNullException when timeProvider is null or restoring the previous timeProvider ??= TimeProvider.System fallback before assigning/using it.
| _timeProvider = timeProvider; | |
| _timeProvider = timeProvider ?? TimeProvider.System; |
The changes enhance SignalR's message buffering by adding a timeout mechanism for backpressure situations and expanding test coverage to verify the new behavior.
src/SignalR/common/Shared/MessageBuffer.cs: Updated backpressure logic to use a 5-second cancellation token with TimeProvider and improved timer initialization.src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs: Added new tests for default and cancellable backpressure writes and renamed an existing test for clarity.src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs: Introduced and adjusted scenarios to test backpressure timeout handling in hub connection scenarios.