From c53514fa19bbb776ee3855a6cfaa1c4706b7f446 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 30 Apr 2018 09:22:48 -0700 Subject: [PATCH] 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 --- .../HubConnectionContext.cs | 25 ++++++++++++------- .../HubConnectionHandler.cs | 11 ++------ .../HubConnectionHandlerTestUtils/Hubs.cs | 22 ++++++++++++++++ .../HubConnectionHandlerTests.cs | 23 +++++++++++++++++ 4 files changed, 63 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs index a522f9a867..80649d47e1 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs @@ -22,7 +22,7 @@ namespace Microsoft.AspNetCore.SignalR { public class HubConnectionContext { - private static readonly Action _abortedCallback = AbortConnection; + private static readonly WaitCallback _abortedCallback = AbortConnection; private readonly ConnectionContext _connectionContext; private readonly ILogger _logger; @@ -271,7 +271,7 @@ namespace Microsoft.AspNetCore.SignalR 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 HandshakeAsync(TimeSpan timeout, IReadOnlyList supportedProtocols, IHubProtocolResolver protocolResolver, @@ -425,15 +425,15 @@ namespace Microsoft.AspNetCore.SignalR 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 @@ namespace Microsoft.AspNetCore.SignalR private static readonly Action _protocolVersionFailed = LoggerMessage.Define(LogLevel.Warning, new EventId(7, "ProtocolVersionFailed"), "Server does not support version {Version} of the {Protocol} protocol."); + private static readonly Action _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 @@ namespace Microsoft.AspNetCore.SignalR { _protocolVersionFailed(logger, protocolName, version, null); } - } + public static void AbortFailed(ILogger logger, Exception exception) + { + _abortFailed(logger, exception); + } + } } } diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs index c48ea19472..513a26b862 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs @@ -126,15 +126,8 @@ namespace Microsoft.AspNetCore.SignalR // 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 { diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs index 9f6189d785..1192a00d72 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs @@ -549,6 +549,28 @@ namespace Microsoft.AspNetCore.SignalR.Tests 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; diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs index ec8119b19c..ed7ba5dd7e 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs @@ -69,6 +69,29 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [Fact] + public async Task OnDisconnectedAsyncTriggersWhenAbortedTokenCallbackThrows() + { + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(); + var connectionHandler = serviceProvider.GetService>(); + + 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() {