server closes connections that have gone silent (#2364)

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`

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<object> _abortCompletedTcs = new TaskCompletionSource<object>(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<byte> _cachedPingMessage;
+        private bool _clientTimeoutActive;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="HubConnectionContext"/> class.
         /// </summary>
         /// <param name="connectionContext">The underlying <see cref="ConnectionContext"/>.</param>
-        /// <param name="keepAliveInterval">The keep alive interval.</param>
+        /// <param name="keepAliveInterval">The keep alive interval. If no messages are sent by the server in this interval, a Ping message will be sent.</param>
         /// <param name="loggerFactory">The logger factory.</param>
-        public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory)
+        /// <param name="clientTimeoutInterval">Clients we haven't heard from in this interval are assumed to have disconnected.</param>
+        public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory, TimeSpan clientTimeoutInterval)
         {
             _connectionContext = connectionContext;
             _logger = loggerFactory.CreateLogger<HubConnectionContext>();
             ConnectionAborted = _connectionAbortedTokenSource.Token;
-            _keepAliveDuration = (int)keepAliveInterval.TotalMilliseconds * (Stopwatch.Frequency / 1000);
+            _keepAliveInterval = keepAliveInterval.Ticks;
+            _clientTimeoutInterval = clientTimeoutInterval.Ticks;
         }
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="HubConnectionContext"/> class.
+        /// </summary>
+        /// <param name="connectionContext">The underlying <see cref="ConnectionContext"/>.</param>
+        /// <param name="keepAliveInterval">The keep alive interval. If no messages are sent by the server in this interval, a Ping message will be sent.</param>
+        /// <param name="loggerFactory">The logger factory.</param>
+        public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory)
+            : this(connectionContext, keepAliveInterval, loggerFactory, HubOptionsSetup.DefaultClientTimeoutInterval) { }
+
         /// <summary>
         /// Gets a <see cref="CancellationToken"/> that notifies when the connection is aborted.
         /// </summary>
@@ -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<IConnectionHeartbeatFeature>()?.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

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

@@ -69,6 +69,7 @@ namespace Microsoft.AspNetCore.SignalR
             // We check to see if HubOptions<THub> 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<string>) ?? 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);

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

@@ -26,6 +26,11 @@ namespace Microsoft.AspNetCore.SignalR
         /// </summary>
         public TimeSpan? KeepAliveInterval { get; set; } = null;
 
+        /// <summary>
+        /// Gets or sets the time window clients have to send a message before the server closes the connection.
+        /// </summary>
+        public TimeSpan? ClientTimeoutInterval { get; set; } = null;
+
         /// <summary>
         /// Gets or sets a collection of supported hub protocol names.
         /// </summary>

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.
     /// </summary>
     /// <typeparam name="THub">The hub type to configure.</typeparam>
-    public class HubOptions<THub> : HubOptions where THub : Hub { }
+    public class HubOptions<THub> : HubOptions where THub : Hub
+    {
+    }
 }

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

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<string> _protocols = new List<string>();
+        internal static TimeSpan DefaultClientTimeoutInterval => TimeSpan.FromSeconds(30);
+
+        private readonly List<string> _defaultProtocols = new List<string>();
 
         public HubOptionsSetup(IEnumerable<IHubProtocol> 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<string>();
-            }
-
             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<string>();
+            }
+
+            foreach (var protocol in _defaultProtocols)
             {
                 options.SupportedProtocols.Add(protocol);
             }

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

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<string> ReadSentTextMessageAsync()
+
+        public async Task<string> 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<string> ReadSentTextMessageAsyncInner()
+        {
             while (true)
             {
                 var result = await Application.Input.ReadAsync();
@@ -144,7 +163,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             }
         }
 
-        public async Task<IList<string>> ReadAllSentMessagesAsync()
+        public async Task<IList<string>> 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;

test/Microsoft.AspNetCore.SignalR.Tests.Utils/HubConnectionContextUtils.cs

@@ -22,7 +22,7 @@ namespace Microsoft.AspNetCore.SignalR.Tests
 
         public static Mock<HubConnectionContext> CreateMock(ConnectionContext connection)
         {
-            var mock = new Mock<HubConnectionContext>(connection, TimeSpan.FromSeconds(15), NullLoggerFactory.Instance) { CallBase = true };
+            var mock = new Mock<HubConnectionContext>(connection, TimeSpan.FromSeconds(15), NullLoggerFactory.Instance, TimeSpan.FromSeconds(15)) { CallBase = true };
             var protocol = new JsonHubProtocol();
             mock.SetupGet(m => m.Protocol).Returns(protocol);
             return mock;

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<HubOptions>(options =>
+                   options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(100)));
+            var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();
+
+            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<HubOptions>(options =>
+                    options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(100)));
+            var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();
+
+            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<HubOptions>(options =>
+                     options.ClientTimeoutInterval = TimeSpan.FromMilliseconds(300)));
+            var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();
+
+            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()
         {