SignalR performance: track groups per connection, remove on disconnect

[alex-jitbit]

Instead of iterating over ALL the connection-groups, which is slow and even introduces a DDoS vector, we remove from groups that are specific to this connection upon disconnect.

Signalr CPU optimization

Similar to RedisHubLifetimeManager the new DefaultHubLifetimeManager now makes every connection track groups per connection. So when a client disconnects there's no need to iterate a huge dictionary of all groups (there can be tens of thousands.

This change should also drastically speed up application shutdown. When you "recycle" application pool on IIS/Windows, or you gracefully end Kestrel process on Linux, or switch nginx-proxy between application instances in a blue-green scenario - you literally get thousands and thousands of "disconnected" events)

Fixes #48249

[davidfowl]

Does it need to be a feature? Can we just add a list of groups to the connection context? We can optimize with our own internal state.

[davidfowl]

Can we also delete RemoveFromDisconnection since it's no longer used?

[alex-jitbit]

@davidfowl

In case you're asking me:

Does it need to be a feature? Can we just add a list of groups to the connection context?

Performance-wise doesn't matter they're both just dictionaries under the hood.

Context.Items is what I used initially. Then I looked at your other implementations of HubLifetimeManager and saw that you mostly use features. So I though it'd be more "Microsofty" this way. I personally think features look better and the code becomes more readable. Context-items involve casting, type-checks and other ugly hacks.

Can we also delete RemoveFromDisconnection since it's no longer used?

What is RemoveFromDisconnection? Do you mean HubGroupList.RemoveDisconnectedConnection? Sure. However, it's public, so removing it might be a breaking change.

[davidfowl]

Performance-wise doesn't matter they're both just dictionaries under the hood.

It's less dictionaries. Seems apt for our default implementation to be as optimized as it could be.

What is RemoveFromDisconnection? Do you mean HubGroupList.RemoveDisconnectedConnection? Sure. However, it's public, so removing it might be a breaking change.

It's an internal type.

[alex-jitbit]

Sure, I'll remove the unused method ASAP, and rewrite it into connection.Context.Items if the reviewers agree on that.

P.S. I forgot to mention that this change also drastically speeds up application shutdown. When you "recycle" application pool on IIS/Windows, or you gracefully end Kestrel process on Linux, or you just switch nginx-proxy to a new application instance in a blue-green scenario (also on Linux) - you literally get thousands and thousands of "disconnect" events. And shutting down a SignalR app process results in CPU load jumping to 100% and stay there for 10-40 seconds (depending on the number of connections and how beefed up the server is). That is why I'm humbly suggesting releasing this change as a 8.0.x patch

[BrennanConroy]

Does it need to be a feature? Can we just add a list of groups to the connection context?

Performance-wise doesn't matter they're both just dictionaries under the hood.

Context.Items is what I used initially. Then I looked at your other implementations of HubLifetimeManager and saw that you mostly use features. So I though it'd be more "Microsofty" this way. I personally think features look better and the code becomes more readable. Context-items involve casting, type-checks and other ugly hacks.

I think he meant making the HashSet an internal property on HubConnectionContext. That way the collection is directly accessed instead of going through the IFeatureCollection dictionary. It also saves the allocation of the GroupTrackerFeature.

[alex-jitbit]

@BrennanConroy oh, got it. Sure, just updated the PR!

[davidfowl]

@BrennanConroy is this good to go? Do we need any other tests?

[BrennanConroy]

Thanks @alex-jitbit! I know the review comments were a bit spread out so thanks for sticking with it. As for backporting the changes I think that's not too bad since the only way to workaround it would be to do some private reflection.

[BrennanConroy]

/backport to release/8.0

[github-actions]

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