Groups.Add within persistent connection fails under high crank load #388

Closed
joshuahayes opened this Issue May 11, 2012 · 7 comments

Projects

None yet

6 participants

@joshuahayes

Running a self hosted server via a persistent connection. Under normal conditions many clients can connect fine. After running crank with 1000 clients a couple hundred in I usually receive the error 'Collection was modified; enumeration operation may not execute.' when invoking Groups.Add(...)

protected override Task OnConnectedAsync(IRequest request, string connectionId) {
        string virtualPath = request.Url.LocalPath.Split(new string[] { "/" }, StringSplitOptions.RemoveEmptyEntries).First();
        return IsOnline(virtualPath, connectionId, () => {
            var cmd = new ConnectCommand {
                Type = NetworkCommandType.ClientConnected,
                Uri = virtualPath,
                SenderId = connectionId
            };
            **Groups.Add(connectionId, virtualPath);**
            _ClientMap.Add(connectionId, virtualPath);
            Route(cmd);
            return base.OnConnectedAsync(request, connectionId);
        });
    }

Stacktrace

   at System.Collections.Generic.HashSet`1.Enumerator.MoveNext()

at System.Linq.Enumerable.d__711.MoveNext()
at SignalR.InProcessMessageBus
1.<>c__DisplayClassf.b__b(IList1 receivedMessages)
at SignalR.InProcessMessageBus
1.Broadcast(String eventKey, InMemoryMessage1 message)
at SignalR.InProcessMessageBus
1.Send(String connectionId, String eventKey, Object value)
at SignalR.Connection.SendMessage(String key, Object value)
at SignalR.Connection.Send(String signal, Object value)
at SignalR.Infrastructure.ConnectionExtensions.SendCommand(IConnection connection, String connectionId, SignalCommand command)
at SignalR.GroupManager.Add(String connectionId, String groupName)
at WorldServer.SignalR.Connections.DataConnection.<>c__DisplayClass4.b__3() in C:\Development\WorldServer\src\WorldServer.Module.Networking\Connections\DataConnection.cs:line 50
at WorldServer.SignalR.Connections.DataConnection.IsOnline(String vp, String connectionId, Func1 func) in C:\Development\WorldServer\src\WorldServer.Module.Networking\Connections\DataConnection.cs:line 144
at WorldServer.SignalR.Connections.DataConnection.OnConnectedAsync(IRequest request, String connectionId) in C:\Development\WorldServer\src\WorldServer.Module.Networking\Connections\DataConnection.cs:line 44
at SignalR.PersistentConnection.<>c__DisplayClass6.<ProcessRequestAsync>b__2()
at SignalR.TaskAsyncHelper.<>c__DisplayClassd
1.b__c()
at SignalR.Transports.ForeverTransport.<>c__DisplayClass4.b__2()
at SignalR.Transports.ForeverTransport.ProcessMessagesImpl(TaskCompletionSource1 taskCompletetionSource, ITransportConnection connection, Action postReceive)
at SignalR.Transports.ForeverTransport.ProcessMessages(ITransportConnection connection, Action postReceive)
at SignalR.Transports.ForeverTransport.<ProcessReceiveRequest>b__3(ITransportConnection c, Action pr)
at SignalR.TaskAsyncHelper.FromMethod[T1,T2,TResult](Func
3 func, T1 arg1, T2 arg2)

@joshuahayes

This was occurring before and after upgrading to 0.5 proper btw.

@cyr

Likely not thread-safe. Use lock?

@shiftkey

Could you use a ConcurrentDictionary to make this code threadsafe?

EDIT: What version are you running? This is the Broadcast method of master and this is the 0.5.2 version.

@frankradocaj

When I access Groups.Add() or Groups.Remove() methods on an IHubContext instance, I randomly get the same error under minimal load (I'm building some chat functionality). I'm running v0.5.0.

Its definitely not thread-safe as I was able to prevent the error by wrapping a lock around Groups.Add() and Groups.Remove() calls.

@andreialecu

I'm getting the same error under minimal load. I have a client calling a method 5-6 times in close succession from JS. The method runs Groups.Add() twice.

I get the Collection was modified error more often than not, on a Groups.Add() line. And what's disturbing is that lock()ing does not help at all, even with a lock, the error still occurs.

My temporary workaround is to do this, to try it 10 times:

                        lock (_lock)
                        {
                            for (int i = 0; i < 10; i++)
                            {
                                try
                                {
                                    Groups.Add(Context.ConnectionId, model.Endpoint);
                                    Groups.Add(Context.ConnectionId, "user:" + model.Name.ToLower());
                                    Thread.Sleep(10);
                                    break;
                                }
                                catch
                                {

                                }
                            }
                        }
@andreialecu

This is the stacktrace:

at System.Collections.Generic.HashSet1.Enumerator.MoveNext()
at System.Linq.Enumerable.<ConcatIterator>d__71
1.MoveNext()
at SignalR.InProcessMessageBus1.<>c__DisplayClassf.<WaitForMessages>b__b(IList1 receivedMessages)
at SignalR.InProcessMessageBus1.Broadcast(String eventKey, InMemoryMessage1 message)
at SignalR.InProcessMessageBus`1.Send(String connectionId, String eventKey, Object value)
at SignalR.Connection.SendMessage(String key, Object value)
at ChatWebApp.Infrastructure.ChatHub.<>c__DisplayClass14.b__11() in S:\Projects\ChatWebApp\ChatWebApp\Infrastructure\ChatHub.cs:line 110
at System.Threading.Tasks.Task.Execute()

@andreialecu

I got the source and fixed it using the suggested fix here:
#537

@davidfowl davidfowl was assigned Jul 24, 2012
@davidfowl davidfowl closed this in 399fe59 Jul 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment