Remove _stateLock from Connection.Stop to prevent deadlocks during Abort

#2191
SignalR · Jul 30, 2013 · e06055a · e06055a
1 parent aacb980
commit e06055a
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 28 deletions.
diff --git a/src/Microsoft.AspNet.SignalR.Client/Connection.cs b/src/Microsoft.AspNet.SignalR.Client/Connection.cs
@@ -499,38 +499,21 @@ public void Stop(TimeSpan timeout)
                         Trace(TraceLevels.Events, "Error: {0}", ex.GetBaseException());
                     }
                 }
-
-                lock (_stateLock)
+
+                // This is racy since it's outside the _stateLock, but we are trying to avoid 30s deadlocks when calling _transport.Abort()
+                if (State == ConnectionState.Disconnected)
                 {
-                    // Do nothing if the connection is offline
-                    if (State != ConnectionState.Disconnected)
-                    {
-                        string connectionId = ConnectionId;
-
-                        Trace(TraceLevels.Events, "Stop");
-
-                        // If we've connected then instantly disconnected we may have data in the incomingMessageBuffer
-                        // Therefore we need to clear it incase we start the connection again.
-                        _connectingMessageBuffer.Clear();
-
-                        // Dispose the heart beat monitor so we don't fire notifications when waiting to abort
-                        _monitor.Dispose();
-
-                        _transport.Abort(this, timeout, _connectionData);
+                    return;
+                }
 
-                        Disconnect();
+                Trace(TraceLevels.Events, "Stop");
 
-                        _disconnectCts.Dispose();
+                // Dispose the heart beat monitor so we don't fire notifications when waiting to abort
+                _monitor.Dispose();
 
-                        if (_transport != null)
-                        {
-                            Trace(TraceLevels.Events, "Transport.Dispose({0})", connectionId);
+                _transport.Abort(this, timeout, _connectionData);
 
-                            _transport.Dispose();
-                            _transport = null;
-                        }
-                    }
-                }
+                Disconnect();
             }
         }
 
@@ -555,10 +538,23 @@ private void Disconnect()
 
                     Trace(TraceLevels.StateChanges, "Disconnected");
 
+                    // If we've connected then instantly disconnected we may have data in the incomingMessageBuffer
+                    // Therefore we need to clear it incase we start the connection again.
+                    _connectingMessageBuffer.Clear();
+
                     _disconnectTimeoutOperation.Dispose();
                     _disconnectCts.Cancel();
+                    _disconnectCts.Dispose();
                     _monitor.Dispose();
 
+                    if (_transport != null)
+                    {
+                        Trace(TraceLevels.Events, "Transport.Dispose({0})", ConnectionId);
+
+                        _transport.Dispose();
+                        _transport = null;
+                    }
+
                     Trace(TraceLevels.Events, "Closed");
 
                     // Clear the state for this connection

diff --git a/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs b/tests/Microsoft.AspNet.SignalR.FunctionalTests/Client/HubProxyFacts.cs
@@ -104,9 +104,11 @@ public void ConnectionFailsStartOnMultipleTransportTimeouts(HostType hostType, T
                         mre.Set();
                     });
 
+                    var transport = hubConnection.Transport;
+
                     // Should take 1-2s per transport timeout
                     Assert.True(mre.Wait(TimeSpan.FromSeconds(15)));
-                    Assert.True(String.IsNullOrEmpty(hubConnection.Transport.Name));
+                    Assert.True(String.IsNullOrEmpty(transport.Name));
                 }
             }
         }