diff --git a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs index f879e03c47..d5237df322 100644 --- a/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs +++ b/src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs @@ -884,11 +884,16 @@ namespace Microsoft.AspNetCore.SignalR.Client private async Task TimerLoop(TimerAwaitable timer) { + // Tell the server we intend to ping + // Old clients never ping, and shouldn't be timed out + // So ping to tell the server that we should be timed out if we stop + await SendHubMessage(PingMessage.Instance); + // initialize the timers timer.Start(); - ResetSendPing(); ResetTimeout(); - + ResetSendPing(); + using (timer) { // await returns True until `timer.Stop()` is called in the `finally` block of `ReceiveLoop` diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs index c19e24b4ff..880e16248a 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs @@ -28,26 +28,41 @@ namespace Microsoft.AspNetCore.SignalR private readonly ILogger _logger; private readonly CancellationTokenSource _connectionAbortedTokenSource = new CancellationTokenSource(); private readonly TaskCompletionSource _abortCompletedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - private readonly long _keepAliveDuration; + private readonly long _keepAliveInterval; + private readonly long _clientTimeoutInterval; private readonly SemaphoreSlim _writeLock = new SemaphoreSlim(1); - private long _lastSendTimestamp = Stopwatch.GetTimestamp(); + private long _lastSendTimeStamp = DateTime.UtcNow.Ticks; + private long _lastReceivedTimeStamp = DateTime.UtcNow.Ticks; + private bool _receivedMessageThisInterval = false; private ReadOnlyMemory _cachedPingMessage; + private bool _clientTimeoutActive; /// /// Initializes a new instance of the class. /// /// The underlying . - /// The keep alive interval. + /// The keep alive interval. If no messages are sent by the server in this interval, a Ping message will be sent. /// The logger factory. - public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory) + /// Clients we haven't heard from in this interval are assumed to have disconnected. + public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory, TimeSpan clientTimeoutInterval) { _connectionContext = connectionContext; _logger = loggerFactory.CreateLogger(); ConnectionAborted = _connectionAbortedTokenSource.Token; - _keepAliveDuration = (int)keepAliveInterval.TotalMilliseconds * (Stopwatch.Frequency / 1000); + _keepAliveInterval = keepAliveInterval.Ticks; + _clientTimeoutInterval = clientTimeoutInterval.Ticks; } + /// + /// Initializes a new instance of the class. + /// + /// The underlying . + /// The keep alive interval. If no messages are sent by the server in this interval, a Ping message will be sent. + /// The logger factory. + public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory) + : this(connectionContext, keepAliveInterval, loggerFactory, HubOptionsSetup.DefaultClientTimeoutInterval) { } + /// /// Gets a that notifies when the connection is aborted. /// @@ -428,13 +443,15 @@ namespace Microsoft.AspNetCore.SignalR private void KeepAliveTick() { - var timestamp = Stopwatch.GetTimestamp(); + var currentTime = DateTime.UtcNow.Ticks; + // Implements the keep-alive tick behavior // Each tick, we check if the time since the last send is larger than the keep alive duration (in ticks). // If it is, we send a ping frame, if not, we no-op on this tick. This means that in the worst case, the // true "ping rate" of the server could be (_hubOptions.KeepAliveInterval + HubEndPoint.KeepAliveTimerInterval), // because if the interval elapses right after the last tick of this timer, it won't be detected until the next tick. - if (timestamp - Interlocked.Read(ref _lastSendTimestamp) > _keepAliveDuration) + + if (currentTime - Volatile.Read(ref _lastSendTimeStamp) > _keepAliveInterval) { // Haven't sent a message for the entire keep-alive duration, so send a ping. // If the transport channel is full, this will fail, but that's OK because @@ -442,10 +459,37 @@ namespace Microsoft.AspNetCore.SignalR // transport is still in the process of sending frames. _ = TryWritePingAsync(); - Interlocked.Exchange(ref _lastSendTimestamp, timestamp); + // We only update the timestamp here, because updating on each sent message is bad for performance + // There can be a lot of sent messages per 15 seconds + Volatile.Write(ref _lastSendTimeStamp, currentTime); } } + internal void StartClientTimeout() + { + if (_clientTimeoutActive) + { + return; + } + _clientTimeoutActive = true; + Features.Get()?.OnHeartbeat(state => ((HubConnectionContext)state).CheckClientTimeout(), this); + } + + private void CheckClientTimeout() + { + // If it's been too long since we've heard from the client, then close this + if (DateTime.UtcNow.Ticks - Volatile.Read(ref _lastReceivedTimeStamp) > _clientTimeoutInterval) + { + if (!_receivedMessageThisInterval) + { + Abort(); + } + + _receivedMessageThisInterval = false; + Volatile.Write(ref _lastReceivedTimeStamp, DateTime.UtcNow.Ticks); + } + } + private static void AbortConnection(object state) { var connection = (HubConnectionContext)state; @@ -464,6 +508,11 @@ namespace Microsoft.AspNetCore.SignalR } } + internal void ResetClientTimeout() + { + _receivedMessageThisInterval = true; + } + private static class Log { // Category: HubConnectionContext diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs index ae4b3d8960..9211fda589 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionHandler.cs @@ -69,6 +69,7 @@ namespace Microsoft.AspNetCore.SignalR // We check to see if HubOptions are set because those take precedence over global hub options. // Then set the keepAlive and handshakeTimeout values to the defaults in HubOptionsSetup incase they were explicitly set to null. var keepAlive = _hubOptions.KeepAliveInterval ?? _globalHubOptions.KeepAliveInterval ?? HubOptionsSetup.DefaultKeepAliveInterval; + var clientTimeout = _hubOptions.ClientTimeoutInterval ?? _globalHubOptions.ClientTimeoutInterval ?? HubOptionsSetup.DefaultClientTimeoutInterval; var handshakeTimeout = _hubOptions.HandshakeTimeout ?? _globalHubOptions.HandshakeTimeout ?? HubOptionsSetup.DefaultHandshakeTimeout; var supportedProtocols = _hubOptions.SupportedProtocols ?? _globalHubOptions.SupportedProtocols; @@ -79,7 +80,7 @@ namespace Microsoft.AspNetCore.SignalR Log.ConnectedStarting(_logger); - var connectionContext = new HubConnectionContext(connection, keepAlive, _loggerFactory); + var connectionContext = new HubConnectionContext(connection, keepAlive, _loggerFactory, clientTimeout); var resolvedSupportedProtocols = (supportedProtocols as IReadOnlyList) ?? supportedProtocols.ToList(); if (!await connectionContext.HandshakeAsync(handshakeTimeout, resolvedSupportedProtocols, _protocolResolver, _userIdProvider, _enableDetailedErrors)) @@ -87,6 +88,8 @@ namespace Microsoft.AspNetCore.SignalR return; } + // -- the connectionContext has been set up -- + try { await _lifetimeManager.OnConnectedAsync(connectionContext); @@ -197,6 +200,8 @@ namespace Microsoft.AspNetCore.SignalR if (!buffer.IsEmpty) { + connection.ResetClientTimeout(); + while (protocol.TryParseMessage(ref buffer, _dispatcher, out var message)) { await _dispatcher.DispatchMessageAsync(connection, message); diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubOptions.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubOptions.cs index 92816897ad..25d997dc4d 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubOptions.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubOptions.cs @@ -26,6 +26,11 @@ namespace Microsoft.AspNetCore.SignalR /// public TimeSpan? KeepAliveInterval { get; set; } = null; + /// + /// Gets or sets the time window clients have to send a message before the server closes the connection. + /// + public TimeSpan? ClientTimeoutInterval { get; set; } = null; + /// /// Gets or sets a collection of supported hub protocol names. /// diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubOptions`T.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubOptions`T.cs index c98d40545c..1d69257bc3 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubOptions`T.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubOptions`T.cs @@ -7,5 +7,7 @@ namespace Microsoft.AspNetCore.SignalR /// Options used to configure the specified hub type instances. These options override globally set options. /// /// The hub type to configure. - public class HubOptions : HubOptions where THub : Hub { } + public class HubOptions : HubOptions where THub : Hub + { + } } diff --git a/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs b/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs index 51468349ea..4f1fc15668 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/Internal/DefaultHubDispatcher.cs @@ -107,7 +107,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal break; case PingMessage _: - // We don't care about pings + connection.StartClientTimeout(); break; // Other kind of message we weren't expecting diff --git a/src/Microsoft.AspNetCore.SignalR.Core/Internal/HubOptionsSetup.cs b/src/Microsoft.AspNetCore.SignalR.Core/Internal/HubOptionsSetup.cs index 2aed136de9..ed6498b964 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/Internal/HubOptionsSetup.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/Internal/HubOptionsSetup.cs @@ -14,26 +14,23 @@ namespace Microsoft.AspNetCore.SignalR.Internal internal static TimeSpan DefaultKeepAliveInterval => TimeSpan.FromSeconds(15); - private readonly List _protocols = new List(); + internal static TimeSpan DefaultClientTimeoutInterval => TimeSpan.FromSeconds(30); + + private readonly List _defaultProtocols = new List(); public HubOptionsSetup(IEnumerable protocols) { foreach (var hubProtocol in protocols) { - _protocols.Add(hubProtocol.Name); + _defaultProtocols.Add(hubProtocol.Name); } } public void Configure(HubOptions options) { - if (options.SupportedProtocols == null) - { - options.SupportedProtocols = new List(); - } - if (options.KeepAliveInterval == null) { - // The default keep - alive interval.This is set to exactly half of the default client timeout window, + // The default keep - alive interval. This is set to exactly half of the default client timeout window, // to ensure a ping can arrive in time to satisfy the client timeout. options.KeepAliveInterval = DefaultKeepAliveInterval; } @@ -43,7 +40,12 @@ namespace Microsoft.AspNetCore.SignalR.Internal options.HandshakeTimeout = DefaultHandshakeTimeout; } - foreach (var protocol in _protocols) + if (options.SupportedProtocols == null) + { + options.SupportedProtocols = new List(); + } + + foreach (var protocol in _defaultProtocols) { options.SupportedProtocols.Add(protocol); } diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs index e3a55351f4..642a897946 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.Protocol.cs @@ -580,10 +580,10 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests { await hubConnection.StartAsync().OrTimeout(); - var firstPing = await connection.ReadSentTextMessageAsync().OrTimeout(); + var firstPing = await connection.ReadSentTextMessageAsync(ignorePings: false).OrTimeout(); Assert.Equal("{\"type\":6}", firstPing); - var secondPing = await connection.ReadSentTextMessageAsync().OrTimeout(); + var secondPing = await connection.ReadSentTextMessageAsync(ignorePings: false).OrTimeout(); Assert.Equal("{\"type\":6}", secondPing); } finally diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs index c927a3d42a..fe979ef5a8 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs +++ b/test/Microsoft.AspNetCore.SignalR.Client.Tests/TestConnection.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; using Microsoft.AspNetCore.SignalR.Protocol; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; namespace Microsoft.AspNetCore.SignalR.Client.Tests { @@ -55,7 +56,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests Application.Input.OnWriterCompleted((ex, _) => { Application.Output.Complete(); - }, + }, null); } @@ -116,9 +117,27 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests { return Application.Output.WriteAsync(bytes).AsTask(); } - public async Task ReadSentTextMessageAsync() + + public async Task ReadSentTextMessageAsync(bool ignorePings = true) { // Read a single text message from the Application Input pipe + + while (true) + { + var result = await ReadSentTextMessageAsyncInner(); + + var receivedMessageType = (int?)JObject.Parse(result)["type"]; + + if (ignorePings && receivedMessageType == HubProtocolConstants.PingMessageType) + { + continue; + } + return result; + } + } + + private async Task ReadSentTextMessageAsyncInner() + { while (true) { var result = await Application.Input.ReadAsync(); @@ -144,7 +163,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests } } - public async Task> ReadAllSentMessagesAsync() + public async Task> ReadAllSentMessagesAsync(bool ignorePings = true) { if (!Disposed.IsCompleted) { @@ -155,7 +174,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests while (true) { - var message = await ReadSentTextMessageAsync(); + var message = await ReadSentTextMessageAsync(ignorePings); if (message == null) { break; diff --git a/test/Microsoft.AspNetCore.SignalR.Tests.Utils/HubConnectionContextUtils.cs b/test/Microsoft.AspNetCore.SignalR.Tests.Utils/HubConnectionContextUtils.cs index d313c2b91c..304d9332e1 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests.Utils/HubConnectionContextUtils.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests.Utils/HubConnectionContextUtils.cs @@ -22,7 +22,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests public static Mock CreateMock(ConnectionContext connection) { - var mock = new Mock(connection, TimeSpan.FromSeconds(15), NullLoggerFactory.Instance) { CallBase = true }; + var mock = new Mock(connection, TimeSpan.FromSeconds(15), NullLoggerFactory.Instance, TimeSpan.FromSeconds(15)) { CallBase = true }; var protocol = new JsonHubProtocol(); mock.SetupGet(m => m.Protocol).Returns(protocol); return mock; diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs index 284153a5ff..659c543e4f 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs @@ -2019,6 +2019,78 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [Fact] + public async Task ConnectionNotTimedOutIfClientNeverPings() + { + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services => + services.Configure(options => + options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(100))); + var connectionHandler = serviceProvider.GetService>(); + + using (var client = new TestClient(new JsonHubProtocol())) + { + var connectionHandlerTask = await client.ConnectAsync(connectionHandler); + await client.Connected.OrTimeout(); + // This is a fake client -- it doesn't auto-ping to signal + + // We go over the 100 ms timeout interval... + await Task.Delay(120); + client.TickHeartbeat(); + + // but client should still be open, since it never pinged to activate the timeout checking + Assert.False(connectionHandlerTask.IsCompleted); + } + } + + [Fact] + public async Task ConnectionTimesOutIfInitialPingAndThenNoMessages() + { + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services => + services.Configure(options => + options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(100))); + var connectionHandler = serviceProvider.GetService>(); + + using (var client = new TestClient(new JsonHubProtocol())) + { + var connectionHandlerTask = await client.ConnectAsync(connectionHandler); + await client.Connected.OrTimeout(); + await client.SendHubMessageAsync(PingMessage.Instance); + + await Task.Delay(300); + client.TickHeartbeat(); + + await Task.Delay(300); + client.TickHeartbeat(); + + await connectionHandlerTask.OrTimeout(); + } + } + + [Fact] + public async Task ReceivingMessagesPreventsConnectionTimeoutFromOccuring() + { + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services => + services.Configure(options => + options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(300))); + var connectionHandler = serviceProvider.GetService>(); + + using (var client = new TestClient(new JsonHubProtocol())) + { + var connectionHandlerTask = await client.ConnectAsync(connectionHandler); + await client.Connected.OrTimeout(); + await client.SendHubMessageAsync(PingMessage.Instance); + + for (int i = 0; i < 10; i++) + { + await Task.Delay(100); + client.TickHeartbeat(); + await client.SendHubMessageAsync(PingMessage.Instance); + } + + Assert.False(connectionHandlerTask.IsCompleted); + } + } + [Fact] public async Task EndingConnectionSendsCloseMessageWithNoError() {