Don't throw from AbortAsync (#2166)

- Log from inside of HubConnectionContext if the user callback failed.
- Use ThreadPool.QUWI instead of Task.Factory.StartNew.
- Remove try catch from HubConnectionHandler

Author: davidfowl

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs

@@ -22,7 +22,7 @@ namespace Microsoft.AspNetCore.SignalR
 {
     public class HubConnectionContext
     {
-        private static readonly Action<object> _abortedCallback = AbortConnection;
+        private static readonly WaitCallback _abortedCallback = AbortConnection;
 
         private readonly ConnectionContext _connectionContext;
         private readonly ILogger _logger;
@@ -271,7 +271,7 @@ public virtual void Abort()
             Input.CancelPendingRead();
 
             // We fire and forget since this can trigger user code to run
-            Task.Factory.StartNew(_abortedCallback, this);
+            ThreadPool.QueueUserWorkItem(_abortedCallback, this);
         }
 
         internal async Task<bool> HandshakeAsync(TimeSpan timeout, IReadOnlyList<string> supportedProtocols, IHubProtocolResolver protocolResolver,
@@ -425,15 +425,15 @@ private static void AbortConnection(object state)
             try
             {
                 connection._connectionAbortedTokenSource.Cancel();
-
-                // Communicate the fact that we're finished triggering abort callbacks
-                connection._abortCompletedTcs.TrySetResult(null);
             }
             catch (Exception ex)
             {
-                // TODO: Should we log if the cancellation callback fails? This is more preventative to make sure
-                // we don't end up with an unobserved task
-                connection._abortCompletedTcs.TrySetException(ex);
+                Log.AbortFailed(connection._logger, ex);
+            }
+            finally
+            {
+                // Communicate the fact that we're finished triggering abort callbacks
+                connection._abortCompletedTcs.TrySetResult(null);
             }
         }
 
@@ -461,6 +461,9 @@ private static class Log
             private static readonly Action<ILogger, string, int, Exception> _protocolVersionFailed =
                 LoggerMessage.Define<string, int>(LogLevel.Warning, new EventId(7, "ProtocolVersionFailed"), "Server does not support version {Version} of the {Protocol} protocol.");
 
+            private static readonly Action<ILogger, Exception> _abortFailed =
+                LoggerMessage.Define(LogLevel.Trace, new EventId(8, "AbortFailed"), "Abort callback failed.");
+
             public static void HandshakeComplete(ILogger logger, string hubProtocol)
             {
                 _handshakeComplete(logger, hubProtocol, null);
@@ -495,7 +498,11 @@ public static void ProtocolVersionFailed(ILogger logger, string protocolName, in
             {
                 _protocolVersionFailed(logger, protocolName, version, null);
             }
-        }
 
+            public static void AbortFailed(ILogger logger, Exception exception)
+            {
+                _abortFailed(logger, exception);
+            }
+        }
     }
 }

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs

@@ -126,15 +126,8 @@ private async Task HubOnDisconnectedAsync(HubConnectionContext connection, Excep
             // We wait on abort to complete, this is so that we can guarantee that all callbacks have fired
             // before OnDisconnectedAsync
 
-            try
-            {
-                // Ensure the connection is aborted before firing disconnect
-                await connection.AbortAsync();
-            }
-            catch (Exception ex)
-            {
-                Log.AbortFailed(_logger, ex);
-            }
+            // Ensure the connection is aborted before firing disconnect
+            await connection.AbortAsync();
 
             try
             {

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs

@@ -549,6 +549,28 @@ public interface IVoidReturningTypedHubClient
         void Send(string message);
     }
 
+    public class ErrorInAbortedTokenHub : Hub
+    {
+        public override Task OnConnectedAsync()
+        {
+            Context.Items[nameof(OnConnectedAsync)] = true;
+
+            Context.ConnectionAborted.Register(() =>
+            {
+                throw new InvalidOperationException("BOOM");
+            });
+
+            return base.OnConnectedAsync();
+        }
+
+        public override Task OnDisconnectedAsync(Exception exception)
+        {
+            Context.Items[nameof(OnDisconnectedAsync)] = true;
+
+            return base.OnDisconnectedAsync(exception);
+        }
+    }
+
     public class ConnectionLifetimeHub : Hub
     {
         private readonly ConnectionLifetimeState _state;

test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs

@@ -69,6 +69,29 @@ public async Task ConnectionAbortedTokenTriggers()
             }
         }
 
+        [Fact]
+        public async Task OnDisconnectedAsyncTriggersWhenAbortedTokenCallbackThrows()
+        {
+            var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider();
+            var connectionHandler = serviceProvider.GetService<HubConnectionHandler<ErrorInAbortedTokenHub>>();
+
+            using (var client = new TestClient())
+            {
+                var connectionHandlerTask = await client.ConnectAsync(connectionHandler);
+
+                // kill the connection
+                client.Dispose();
+
+                await connectionHandlerTask.OrTimeout();
+
+                var firedOnConnected = (bool)client.Connection.Items[nameof(ErrorInAbortedTokenHub.OnConnectedAsync)];
+                var firedOnDisconnected = (bool)client.Connection.Items[nameof(ErrorInAbortedTokenHub.OnDisconnectedAsync)];
+
+                Assert.True(firedOnConnected);
+                Assert.True(firedOnDisconnected);
+            }
+        }
+
         [Fact]
         public async Task AbortFromHubMethodForcesClientDisconnect()
         {