From 323dae4ce937b15a5e3db246204ed5b57394c0a5 Mon Sep 17 00:00:00 2001
From: David Fowler <davidfowl@gmail.com>
Date: Sun, 21 May 2017 23:19:17 -0700
Subject: [PATCH] Handle case where scanning happens after dispose. (#475)

- ConnectionManager.Scan would null ref because the timer.Change was being called
after closing all connections.
---
 .../ConnectionManager.cs                            | 13 ++++++-------
 .../ConnectionManagerTests.cs                       | 11 +++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/Microsoft.AspNetCore.Sockets/ConnectionManager.cs b/src/Microsoft.AspNetCore.Sockets/ConnectionManager.cs
index 8148fdebab..20b3d1c866 100644
--- a/src/Microsoft.AspNetCore.Sockets/ConnectionManager.cs
+++ b/src/Microsoft.AspNetCore.Sockets/ConnectionManager.cs
@@ -66,8 +66,7 @@ namespace Microsoft.AspNetCore.Sockets
 
         public void RemoveConnection(string id)
         {
-            ConnectionState state;
-            if (_connections.TryRemove(id, out state))
+            if (_connections.TryRemove(id, out _))
             {
                 // Remove the connection completely
                 _logger.LogDebug("Removing {connectionId} from the list of connections", id);
@@ -85,9 +84,9 @@ namespace Microsoft.AspNetCore.Sockets
             ((ConnectionManager)state).Scan();
         }
 
-        private void Scan()
+        public void Scan()
         {
-            // If we couldn't get the lock it means one of 2 things is true:
+            // If we couldn't get the lock it means one of 2 things are true:
             // - We're about to dispose so we don't care to run the scan callback anyways.
             // - The previous Scan took long enough that the next scan tried to run in parallel
             // In either case just do nothing and end the timer callback as soon as possible
@@ -132,12 +131,12 @@ namespace Microsoft.AspNetCore.Sockets
                         var ignore = DisposeAndRemoveAsync(c.Value);
                     }
                 }
+
+                // Resume once we finished processing all connections
+                _timer.Change(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
             }
             finally
             {
-                // Resume once we finished processing all connections
-                _timer.Change(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
-
                 // Exit the lock now
                 Monitor.Exit(_executionLock);
             }
diff --git a/test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs b/test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs
index 46df967d5e..4b8f3d32d1 100644
--- a/test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs
+++ b/test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs
@@ -181,6 +181,17 @@ namespace Microsoft.AspNetCore.Sockets.Tests
             Assert.Equal(ConnectionState.ConnectionStatus.Disposed, state.Status);
         }
 
+        [Fact]
+        public void ScanAfterDisposeNoops()
+        {
+            var connectionManager = CreateConnectionManager();
+            var state = connectionManager.CreateConnection();
+
+            connectionManager.CloseConnections();
+
+            connectionManager.Scan();
+        }
+
         private static ConnectionManager CreateConnectionManager()
         {
             return new ConnectionManager(new Logger<ConnectionManager>(new LoggerFactory()));