Skip to content
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

Fix preserving messages for stateful reconnect with backplane #60900

Merged
merged 2 commits into from
Apr 7, 2025

Conversation

BrennanConroy
Copy link
Member

Fixes #55575

Changed MessageBuffer.WriteAsync methods to use the hub message type since that's the only information we're using in the method. And updated the SerializedHubMessage usage to assume we're sending a HubInvocationMessage since that's the only message type we currently use with SerializedHubMessage.

Updated tests:

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Mar 12, 2025
@BrennanConroy BrennanConroy requested a review from halter73 as a code owner March 12, 2025 21:40
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issues with preserving messages during a stateful reconnect in a backplane scenario by updating the MessageBuffer write logic and adding comprehensive tests.

  • Updated MessageBuffer.WriteAsync methods to default to HubInvocationMessage for SerializedHubMessage.
  • Added a new E2E test to simulate ungraceful WebSocket closure and verify stateful reconnect.
  • Introduced a StatefulHub and integrated it into the test startup configuration.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/SignalR/server/StackExchangeRedis/test/RedisEndToEnd.cs Added a new E2E test for stateful reconnect with a custom WebSocketWrapper.
src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs Added a regression test to validate that unacknowledged serialized messages are resent on reconnect.
src/SignalR/server/StackExchangeRedis/test/StatefulHub.cs New hub implementation to support stateful reconnect testing.
src/SignalR/server/StackExchangeRedis/test/Startup.cs Updated endpoints mapping to support the new StatefulHub with stateful reconnect enabled.
src/SignalR/server/Core/src/SerializedHubMessage.cs Added a Debug.Assert to ensure the message type is compatible with HubInvocationMessage.
src/SignalR/common/Shared/MessageBuffer.cs Modified WriteAsync methods to use the message type instead of the message instance for better type clarity.
Comments suppressed due to low confidence (1)

src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs:183

  • The use of square brackets for the InvocationMessage arguments appears to be a syntax error. Consider replacing [1] with a proper array initializer (e.g. new object[] { 1 }).
var invocationMessage = new SerializedHubMessage([new SerializedMessage(protocol.Name, protocol.GetMessageBytes(new InvocationMessage("method1", [1])))]);

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 21, 2025
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to backport this?

@BrennanConroy
Copy link
Member Author

Yeah, looks like a decent number of people have hit this.

@BrennanConroy BrennanConroy merged commit 8491a27 into main Apr 7, 2025
27 checks passed
@BrennanConroy BrennanConroy deleted the brecon/statefulredis branch April 7, 2025 21:28
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview4 milestone Apr 7, 2025
@BrennanConroy
Copy link
Member Author

/backport to release/9.0

@BrennanConroy
Copy link
Member Author

/backport to release/8.0

Copy link
Contributor

github-actions bot commented Apr 7, 2025

Started backporting to release/9.0: https://github.com/dotnet/aspnetcore/actions/runs/14319862207

Copy link
Contributor

github-actions bot commented Apr 7, 2025

Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/14319863946

Copy link
Contributor

github-actions bot commented Apr 7, 2025

@BrennanConroy backporting to "release/8.0" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix preserving messages for stateful reconnect with backplane
Using index info to reconstruct a base tree...
A	src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/Internal/MessageBufferTests.cs
M	src/SignalR/server/StackExchangeRedis/test/RedisEndToEnd.cs
Falling back to patching base and 3-way merge...
Auto-merging src/SignalR/server/StackExchangeRedis/test/RedisEndToEnd.cs
CONFLICT (content): Merge conflict in src/SignalR/server/StackExchangeRedis/test/RedisEndToEnd.cs
Auto-merging src/SignalR/server/SignalR/test/Internal/MessageBufferTests.cs
CONFLICT (content): Merge conflict in src/SignalR/server/SignalR/test/Internal/MessageBufferTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix preserving messages for stateful reconnect with backplane
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stateful reconnect doesn't work as expected when using Redis backplane
2 participants