diff --git a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs index 439311626f..5905722050 100644 --- a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs +++ b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs @@ -97,7 +97,18 @@ namespace Microsoft.AspNetCore.SignalR.Client if (_needKeepAlive) { _logger.ResettingKeepAliveTimer(); - _timeoutTimer.Change(ServerTimeout, Timeout.InfiniteTimeSpan); + + // If the connection is disposed while this callback is firing, or if the callback is fired after dispose + // (which can happen because of some races), this will throw ObjectDisposedException. That's OK, because + // we don't need the timer anyway. + try + { + _timeoutTimer.Change(ServerTimeout, Timeout.InfiniteTimeSpan); + } + catch (ObjectDisposedException) + { + // This is OK! + } } } @@ -157,8 +168,10 @@ namespace Microsoft.AspNetCore.SignalR.Client private async Task DisposeAsyncCore() { - _timeoutTimer.Dispose(); await _connection.DisposeAsync(); + + // Dispose the timer AFTER shutting down the connection. + _timeoutTimer.Dispose(); } // TODO: Client return values/tasks? diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs index 3434658038..59d13a9244 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs @@ -207,6 +207,21 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests Assert.Equal("Server timeout (100.00ms) elapsed without receiving a message from the server.", exception.Message); } + [Fact] + public async Task OnReceivedAfterTimerDisposedDoesNotThrow() + { + var connection = new TestConnection(); + var hubConnection = new HubConnection(connection, new JsonHubProtocol(), new LoggerFactory()); + await hubConnection.StartAsync().OrTimeout(); + await hubConnection.DisposeAsync().OrTimeout(); + + // Fire callbacks, they shouldn't fail + foreach (var registration in connection.Callbacks) + { + await registration.InvokeAsync(new byte[0]); + } + } + // Moq really doesn't handle out parameters well, so to make these tests work I added a manual mock -anurse private class MockHubProtocol : IHubProtocol { diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs index 16b37fb79a..d1107bf394 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs @@ -40,7 +40,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests private bool _closed; private object _closedLock = new object(); - private readonly List _callbacks = new List(); + public List Callbacks { get; } = new List(); public IFeatureCollection Features { get; } = new FeatureCollection(); @@ -130,9 +130,9 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests while (_receivedMessages.Reader.TryRead(out var message)) { ReceiveCallback[] callbackCopies; - lock (_callbacks) + lock (Callbacks) { - callbackCopies = _callbacks.ToArray(); + callbackCopies = Callbacks.ToArray(); } foreach (var callback in callbackCopies) @@ -170,14 +170,14 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests public IDisposable OnReceived(Func callback, object state) { var receiveCallBack = new ReceiveCallback(callback, state); - lock (_callbacks) + lock (Callbacks) { - _callbacks.Add(receiveCallBack); + Callbacks.Add(receiveCallBack); } - return new Subscription(receiveCallBack, _callbacks); + return new Subscription(receiveCallBack, Callbacks); } - private class ReceiveCallback + public class ReceiveCallback { private readonly Func _callback; private readonly object _state;