Deduplicate group messages across connections #61810

datwelk · 2025-05-06T08:51:05Z

Deduplicate SignalR group messages across connections

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This change modifies DefaultHubLifetimeManager and RedisHubLifetimeManager to deduplicate SignalR group messages across connections. Previously, when a connection A is part of N-groups, and a SignalR message is sent to those N-groups, connection A will receive those messages N times. This changes instead checks which connections need to be addressed for a list of groups, and uniques them.

Fixes #61809

dotnet-policy-service · 2025-05-06T08:51:25Z

Thanks for your PR, @@datwelk. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

datwelk · 2025-05-06T09:28:50Z

@dotnet-policy-service agree company="LIVEOP B.V."

gfoidl · 2025-05-06T10:03:26Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

+        List<Task>? tasks = new List<Task>();
+
+        var connections = new ConcurrentDictionary<string, HubConnectionContext>();


I know too little about the code in order to tell if there's an alternative approch for the fix.
But I'm bothered about these allocation. So I guess they should be cached an re-used. In a quite simple manner that could be done as something like:

public class Foo { private static List<Task>? s_tasks; private static ConcurrentDictionary<string, HubConnectionContext>? s_connections; public Task SendAsync() { List<Task> tasks = Interlocked.Exchange(ref s_tasks, null) ?? []; ConcurrentDictionary<string, HubConnectionContext> connections = Interlocked.Exchange(ref s_connections, null) ?? []; try { // Use task and connections return Task.WhenAll(tasks); } finally { tasks.Clear(); connections.Clear(); Interlocked.CompareExchange(ref s_tasks, tasks, null); Interlocked.CompareExchange(ref s_connections, connections, null); } } }

gfoidl · 2025-05-07T08:24:05Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

+                    if (connections == null)
+                    {
+                        connections = new HashSet<string>();
+                    }
+                    connections.Add(connection.Key);


FYI this could be written also as 1-liner.

Suggested change

if (connections == null)

{

connections = new HashSet<string>();

}

connections.Add(connection.Key);

(connections ?? []).Add(connection.Key);

For me it's readability is the same.

In L239 can you also check if the group has items?
If so, the check for connections == null can be removed altogehter, and initialize the HashSet just before the foreach-loop in L241, because if the group has items, then Add is called unconditionally.

The HashSet could then be also initialized with the proper capacity, to avoid potential resizing and copying of the internal structures of HashSet.

gfoidl · 2025-05-07T08:38:55Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

@@ -226,7 +226,7 @@ public override Task SendGroupsAsync(IReadOnlyList<string> groupNames, string me
    {
        // Each task represents the list of tasks for each of the writes within a group
        List<Task>? tasks = null;
-        SerializedHubMessage? message = null;
+        HashSet<string>? connections = null;


Depending on how much we want to put into perf / allocations a few thoughts:

cache the HashSet? See the previous comment above for an simple approach -- but this is also no silver bullet, as it has (in the long term) an Gen2 -> Gen0 reference

HashSet is O(1), but still a bit costly due to hash computation, bucket lookup, etc. So for a small amount of connections a simple array with a linear scan (O(n)) could be faster -- the logic could be encapsulated in a new type "FastHashSet" which starts with the array-approach and switches to the HashSet when more items are present

Well, if that's a goal this can be done in a separate PR for sure.

gfoidl · 2025-05-07T08:40:35Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

+                if (tasks == null)
+                {
+                    tasks = new List<Task>();
+                }


HashSet has a Count property, so initialize the tasks list to proper size before the loop, thus avoiding this if.

gfoidl · 2025-05-07T08:42:53Z

src/SignalR/server/StackExchangeRedis/src/RedisHubLifetimeManager.cs

+                    {
+                        foreach (var connection in connectionStore)
+                        {
+                            if (connections == null)


Can similar optimizations as above be applied here?

And to the ToList() call below. I wonder if we shouldn't be using a pooled Span instead.

halter73

Can you explain more details about the design of your app where you're broadcasting a message to multiple groups that a single client might be subscribed to? Might it be better to have these clients subscribe to a separate group meant solely for the previously multi-group broadcasts? Even after this change, that would be more efficient.

This does seem like a nicer behavior than before. I can't think of a reason you'd want a client to receive duplicate messages just because they're in multiple groups the original message was sent to, but I'm not sure it's worth the extra complexity and performance hit if no one has complained since yesterday. However, I'm fine taking it if the perf impact is small.

@BrennanConroy @sebastienros How hard would it be to measure the perf impact of this change?

halter73 · 2025-05-07T19:32:16Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

+                {
+                    tasks = new List<Task>();
+                }
+                tasks.Add(SendConnectionAsync(connectionId, methodName, args, cancellationToken));


This call to SendConnectionAsync reserializes the method invocation payload for each connection.

aspnetcore/src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

Lines 194 to 196 in aa2e0dc

// We're sending to a single connection

// Write message directly to connection without caching it in memory

var message = CreateInvocationMessage(methodName, args);

Prior to this change, SendToGroupConnections would call CreateSerializedInvocationMessage once, send the same serialized invocation payload to each client. I suspect that will have a far bigger performance impact than the HashSet, although I'd also like to pool that if we move forward with this change.

halter73 · 2025-05-07T19:37:40Z

src/SignalR/server/Core/src/DefaultHubLifetimeManager.cs

@@ -238,7 +238,26 @@ public override Task SendGroupsAsync(IReadOnlyList<string> groupNames, string me
            var group = _groups[groupName];
            if (group != null)
            {
-                DefaultHubLifetimeManager<THub>.SendToGroupConnections(methodName, args, group, null, null, ref tasks, ref message, cancellationToken);
+                foreach (var connection in group)


Nit:

Suggested change

foreach (var connection in group)

foreach (var (connectionId, _) in group)

halter73 · 2025-05-07T19:39:27Z

src/SignalR/server/StackExchangeRedis/src/RedisHubLifetimeManager.cs

+                    {
+                        foreach (var connection in connectionStore)
+                        {
+                            if (connections == null)


And to the ToList() call below. I wonder if we shouldn't be using a pooled Span instead.

dotnet-policy-service · 2025-05-15T04:39:55Z

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.

datwelk · 2025-05-15T08:50:20Z

@halter73 Thank you for your time and feedback!

I came across an earlier reference to a similar issue from 2019: #14782. One suggestion there was to add a distinct parameter to the method - e.g., SendGroupsAsync(..., bool distinct)- or a separate method - e.g. SendGroupsDistinctAsync(..) - to avoid potential performance regressions for existing usage.

Let's say we have an entity called fruit, and users can subscribe to updates for all fruits, individual fruit types, or both. To keep the client simple, we don’t prevent overlapping subscriptions - e.g., subscribing to both “all fruits” and “oranges”. This means a single orange update may be received multiple times as it's broadcast to the "all fruits" and "oranges" groups.

While we could try to handle this on the client side, doing so introduces significant complexity. For instance, if a user subscribes to “all fruits” but disables one type (like kiwi), we’d have to unsubscribe from “all fruits” and then individually subscribe to every other fruit type. Managing this becomes more cumbersome as more fruit types - or more entities in general - are added to the app.

If introducing a distinct parameter and modifying the public API is the preferred approach, I’d be happy to proceed with that. Alternatively, I can implement the previously suggested workaround—please let me know your preference.

ghost added the area-signalr Includes: SignalR clients and servers label May 6, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2025

datwelk marked this pull request as ready for review May 6, 2025 09:29

datwelk requested review from BrennanConroy and halter73 as code owners May 6, 2025 09:29

gfoidl reviewed May 6, 2025

View reviewed changes

datwelk requested a review from captainsafia as a code owner May 6, 2025 16:50

deduplicate group messages across connections

c393463

datwelk force-pushed the feature/deduplicate-group-messages-across-connections branch from 29e1aa1 to c393463 Compare May 7, 2025 07:08

gfoidl reviewed May 7, 2025

View reviewed changes

halter73 reviewed May 7, 2025

View reviewed changes

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate group messages across connections #61810

Deduplicate group messages across connections #61810

		List<Task>? tasks = new List<Task>();

		var connections = new ConcurrentDictionary<string, HubConnectionContext>();

	// We're sending to a single connection
	// Write message directly to connection without caching it in memory
	var message = CreateInvocationMessage(methodName, args);

	foreach (var connection in group)
	foreach (var (connectionId, _) in group)

Deduplicate group messages across connections #61810

Are you sure you want to change the base?

Deduplicate group messages across connections #61810

Conversation

Deduplicate SignalR group messages across connections

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment