Skip to content

Avoid client result invocation ID collisions #43716

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

Merged
merged 2 commits into from
Sep 7, 2022
Merged

Conversation

BrennanConroy
Copy link
Member

Fixes bullet 7 of #5280

Stephen had an interesting idea:

Couldn't we pass in a per-connection, IInvocationBinder that adds the connection id prefix back to the invocation id so the IHubProtocol doesn't need to be aware of it?

Unfortunately, we don't know the connection ID when IHubProtocol queries IInvocationBinder so the invocation id has to be the entire source of truth for the lookup.

After doing some local perf testing, the best approach I found was to just generate a GUID and base64 encode it for every invocation. There is only one allocation, the final string, and GUID generation is still fairly cheap, the most expensive part is likely getting the random bytes.

Other alternatives considered:

  • generate a shorter random prefix at startup and append an incrementing ID every invoke. Then every 1000 or so invocations we generate a new random prefix and reset the ID to 0 (so the full invocation ID doesn't grow too big). Sadly, I couldn't figure out how to do this lock free, or even lock free until it needed to reset. So this approach becomes very slow in a multi-threaded environment.
  • generate a shorter random prefix at startup and append an incrementing ID every invoke. If a collision occurs, reply to the original server that it collided and original server will try to use a new ID, repeat. This approach would require more noise over the backplane and complexities with coding.
  • Use some form of randomness and then just throw if a collision occurs. Collisions should be rare and application code likely already is handling exceptions because of timeouts, connection closing, connection throwing error for a result, etc.

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Sep 2, 2022
@BrennanConroy BrennanConroy added this to the 7.0-rc2 milestone Sep 2, 2022
@davidfowl
Copy link
Member

Do you have any of the perf results? This is something we can change in the future if performance is a problem right?

@@ -42,6 +42,11 @@ public void AddInvocation(string invocationId, (Type Type, string ConnectionId,
{
var result = _pendingInvocations.TryAdd(invocationId, invocationInfo);
Debug.Assert(result);
// Should have a 50% chance of happening once every 2.71 quintillion invocations (see UUID in Wikipedia)
Copy link
Member

@davidfowl davidfowl Sep 4, 2022

Choose a reason for hiding this comment

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

🤣 . Are we taking bets on if we ever see this error message filed in an issue?

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Sep 7, 2022

This is something we can change in the future if performance is a problem right?

Yes, it's an internal implementation detail. Although if we wanted to do something co-operative it could be breaking if old server talks to new server.

100 threads, each thread doing 1,000,000 invocations:

lock new id every time hardcoded "random" prefix and increment id every time
21.3158182s 2.9508009s 3.6222541s

@BrennanConroy
Copy link
Member Author

@Pilchie for RC2. This fixes a bug when using multiple servers and the redis backplane in SignalR in combination with the new client result feature where it was possible for 2 IDs to collide which would cause the caller to hang in their app code until the cancellation token (they hopefully passed in) is canceled.

@Pilchie
Copy link
Member

Pilchie commented Sep 7, 2022

Approved for .NET 7 RC2.

@BrennanConroy BrennanConroy merged commit baff28b into release/7.0 Sep 7, 2022
@BrennanConroy BrennanConroy deleted the brecon/redisid branch September 7, 2022 22:30
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 this pull request may close these issues.

3 participants