Signalr AddToGroupAsync possible race condition

[maknapp]

Describe the bug

I was just looking at how groups are managed in Signalr and have not actually seen this happen. If a connection was disconnecting while something else was adding it to a group, would the following be possible?

Lines 47-51 are executed - The connection to add exists
Lines 299-300 are executed on another thread - The connection is removed from connection list and all groups
Line 53 is executed - The connection is added to the group but that connection is no longer in the connection list

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

Lines 47 to 53 in cfe6f52

	var connection = _connections[connectionId];
	if (connection == null)
	{
	return Task.CompletedTask;
	}
	_groups.Add(connection, groupName);

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

Lines 297 to 302 in cfe6f52

	public override Task OnDisconnectedAsync(HubConnectionContext connection)
	{
	_connections.Remove(connection);
	_groups.RemoveDisconnectedConnection(connection.ConnectionId);
	return Task.CompletedTask;
	}

If this is an issue, would another check for _connections[connectionId] is null after adding the connection to the group (after line 53) solve the issue?

To Reproduce

None

Exceptions (if any)

None

Further technical details

• ASP.NET Core version: 6

[davidfowl]

Yes this looks like a valid race condition.

If this is an issue, would another check for _connections[connectionId] is null after adding the connection to the group (after line 53) solve the issue?

That would make the race even less likely but it wouldn't fix it. The first things that come to mind are:

1. Add a single lock here (but that would hurt the performance)
2. Add a lock per connection and avoid removing state while the connection is "in use".
3. Add a connection state so you can detect this scenario and avoid the clean up if you're in the middle of a group add (similar to 2 but lock free)

cc @BrennanConroy for ideas

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[BrennanConroy]

Add a connection state so you can detect this scenario and avoid the clean up if you're in the middle of a group add (similar to 2 but lock free)

We could use the HubConnectionContext.ConnectionAborted token since it will be canceled before OnDisconnectedAsync is called. Check before and after adding to the group, remove from group if we added to it and ConnectionAborted was triggered. The only race that should be left is OnDisconnectedAsync removing the group and AddToGroupAsync removing the group, but that case is thread-safe.

[maknapp]

My idea was to check _connections[connectionId] after adding to the group and then removing it from the group if connection was null. I have been trying to identify the (less likely) race condition for that and cannot - can someone point that out? The worst case I see is trying to remove from the group twice, which would not be a problem.

How is checking _connections before and after _groups.Add different from checking HubConnectionContext.ConnectionAborted before and after?