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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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])))]);
Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@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
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
1 participant