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

Stateful reconnect doesn't work as expected when using Redis backplane #55575

Closed
1 task done
ruben-ivre opened this issue May 7, 2024 · 4 comments · Fixed by #60900
Closed
1 task done

Stateful reconnect doesn't work as expected when using Redis backplane #55575

ruben-ivre opened this issue May 7, 2024 · 4 comments · Fixed by #60900
Labels
area-signalr Includes: SignalR clients and servers

Comments

@ruben-ivre
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Stateful reconnect requires that both client and server calculate the same sequence Id so when the reconnect happen they can resume on the right message, but when using Redis as backplane a different code path is used, which ends up causing some messages not being accounted for in the server side and creating a discrepancy that leads to lost messages on reconnect.
The whole sequence Id calculation happens in MessageBuffer class, which increases the Id every time an invocation message is sent (in WriteAsyncCore), skipping the count otherwise.

The problem is that when using Redis, the message comes as a SerializedHubMessage using a constructor that doesn't set the Message property, so when MessageBuffer tries to determine if it's an invocation message, is operator returns false as the message is null, and thus this message doesn't increment the sequence Id.
On the client side is read correctly, identified as an Invocation, and increments the sequence Id, create a discrepancy that will essentially break stateful reconnect.

Expected Behavior

Sequence Id is kept in sync in both client side and server side. This is critical for stateful reconnect to work properly.

Steps To Reproduce

  1. Create a new SignalR core project, with Redis backplane.
  2. Enable stateful reconnect on both client and server.
  3. Send a message to all users (or a group), so it's sent through the backplane.
  4. Observe how _totalMessageCount in MessageBuffer in server side is not incremented with SerializedHubMessage coming from Redis backplane.

Exceptions (if any)

No response

.NET Version

8.0.3

Anything else?

The issue is pretty easy to spot once you know where to look.

Observe here how MessageBuffer.cs:124 expects Message property to be not null.
image

Observe here the constructor in SerializedHubMessage.cs:27 that is used by Redis backplane. No assignment of Message property.
image

Observe here how MessageBuffer.cs:161 checks message using is operator that will yield false as it is null, not incrementing the sequence Id as it should have.
image

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-signalr Includes: SignalR clients and servers label May 7, 2024
@ruben-ivre
Copy link
Author

ruben-ivre commented May 9, 2024

So far I have a workaround it that is to change that WriteAsync function to create an empty invocation message if hubMessage.Message is null, which works for my use case, but it's not a very good solution.
image

Probably a better option would be to add some info to SerializedHubMessage to be able to reconstruct the proper message type. If you can point me to a proper solution, I can work on that.

@VakhrameevAnton
Copy link

We have the same issue. Any updates?

@davidfowl
Copy link
Member

cc @BrennanConroy

@BrennanConroy
Copy link
Member

That's unfortunate. Seems like we should just pass the hub message type into MessageBuffer.WriteAsyncCore instead of the HubMessage itself since we're not actually using anything from HubMessage except to look at what type it is.

We might want to in the future refactor some of the SerializeHubMessage stuff to be explicit about message types, but for now I think a quick fix would be to change MessageBuffer to use hubMessage.GetType() and always pass typeof(HubInvocationMessage) for the SerializeHubMessage overload, since we know it's always an invocation type or stream item type.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants