React to aspnet/KestrelHttpServer#2496: make IConnectionInherentKeepAliveFeature a boolean feature (#2041)

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/DefaultHubLifetimeManagerBenchmark.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -122,4 +122,4 @@ namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
             return _hubLifetimeManager.SendUsersAsync(_userIdentifiers, "MethodName", Array.Empty<object>());
         }
     }
-}
+}

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/HubConnectionStartBenchmark.cs

@@ -76,6 +76,6 @@ namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
 
     public class TestConnectionInherentKeepAliveFeature : IConnectionInherentKeepAliveFeature
     {
-        public TimeSpan KeepAliveInterval { get; } = TimeSpan.Zero;
+        public bool HasInherentKeepAlive { get; } = true;
     }
 }

benchmarks/Microsoft.AspNetCore.SignalR.Microbenchmarks/Shared/TestConnectionInherentKeepAliveFeature.cs

@@ -1,13 +0,0 @@
-// Copyright (c) .NET Foundation. All rights reserved.
-// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
-
-using System;
-using Microsoft.AspNetCore.Connections.Features;
-
-namespace Microsoft.AspNetCore.SignalR.Microbenchmarks.Shared
-{
-    public class TestConnectionInherentKeepAliveFeature : IConnectionInherentKeepAliveFeature
-    {
-        public TimeSpan KeepAliveInterval { get; } = TimeSpan.Zero;
-    }
-}

build/dependencies.props

@@ -1,4 +1,4 @@
-<Project>
+<Project>
   <PropertyGroup>
     <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
   </PropertyGroup>
@@ -14,7 +14,7 @@
     <MicrosoftAspNetCoreAuthorizationPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreAuthorizationPackageVersion>
     <MicrosoftAspNetCoreAuthorizationPolicyPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreAuthorizationPolicyPackageVersion>
     <MicrosoftAspNetCoreBenchmarkRunnerSourcesPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreBenchmarkRunnerSourcesPackageVersion>
-    <MicrosoftAspNetCoreConnectionsAbstractionsPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreConnectionsAbstractionsPackageVersion>
+    <MicrosoftAspNetCoreConnectionsAbstractionsPackageVersion>2.1.0-a-preview3-inherent-keep-alive-bool-17670</MicrosoftAspNetCoreConnectionsAbstractionsPackageVersion>
     <MicrosoftAspNetCoreCorsPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreCorsPackageVersion>
     <MicrosoftAspNetCoreDiagnosticsEntityFrameworkCorePackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreDiagnosticsEntityFrameworkCorePackageVersion>
     <MicrosoftAspNetCoreDiagnosticsPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreDiagnosticsPackageVersion>
@@ -28,7 +28,7 @@
     <MicrosoftAspNetCoreRoutingPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreRoutingPackageVersion>
     <MicrosoftAspNetCoreServerIISIntegrationPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreServerIISIntegrationPackageVersion>
     <MicrosoftAspNetCoreServerIntegrationTestingPackageVersion>0.5.0-preview2-32233</MicrosoftAspNetCoreServerIntegrationTestingPackageVersion>
-    <MicrosoftAspNetCoreServerKestrelPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreServerKestrelPackageVersion>
+    <MicrosoftAspNetCoreServerKestrelPackageVersion>2.1.0-a-preview3-inherent-keep-alive-bool-17670</MicrosoftAspNetCoreServerKestrelPackageVersion>
     <MicrosoftAspNetCoreStaticFilesPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreStaticFilesPackageVersion>
     <MicrosoftAspNetCoreTestHostPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreTestHostPackageVersion>
     <MicrosoftAspNetCoreTestingPackageVersion>2.1.0-preview3-32233</MicrosoftAspNetCoreTestingPackageVersion>

samples/ClientSample/Tcp/TcpConnection.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.IO.Pipelines;
@@ -40,7 +40,8 @@ namespace ClientSample
         public override string ConnectionId { get; set; } = Guid.NewGuid().ToString();
         public override IDictionary<object, object> Items { get; set; } = new ConnectionItems();
 
-        public TimeSpan KeepAliveInterval => Timeout.InfiniteTimeSpan;
+        // We claim to have inherent keep-alive so the client doesn't kill the connection when it hasn't seen ping frames.
+        public bool HasInherentKeepAlive { get; } = true;
 
         public Task DisposeAsync()
         {

src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs

@@ -12,14 +12,13 @@ using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Connections.Features;
 using Microsoft.AspNetCore.Http.Connections.Client.Internal;
-using Microsoft.AspNetCore.Http.Connections.Features;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 
 namespace Microsoft.AspNetCore.Http.Connections.Client
 {
-    public partial class HttpConnection : ConnectionContext
+    public partial class HttpConnection : ConnectionContext, IConnectionInherentKeepAliveFeature
     {
         private static readonly TimeSpan HttpClientTimeout = TimeSpan.FromSeconds(120);
 #if !NETCOREAPP2_1
@@ -31,6 +30,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
         private readonly SemaphoreSlim _connectionLock = new SemaphoreSlim(1, 1);
         private bool _started;
         private bool _disposed;
+        private bool _hasInherentKeepAlive;
 
         private readonly HttpClient _httpClient;
         private readonly HttpConnectionOptions _httpConnectionOptions;
@@ -59,6 +59,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
         public override string ConnectionId { get; set; }
         public override IDictionary<object, object> Items { get; set; } = new ConnectionItems();
 
+        bool IConnectionInherentKeepAliveFeature.HasInherentKeepAlive => _hasInherentKeepAlive;
+
         public HttpConnection(Uri url)
             : this(url, HttpTransports.All)
         { }
@@ -102,6 +104,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
             _transportFactory = new DefaultTransportFactory(httpConnectionOptions.Transports, _loggerFactory, _httpClient, httpConnectionOptions);
             _logScope = new ConnectionLogScope();
             _scopeDisposable = _logger.BeginScope(_logScope);
+
+            Features.Set<IConnectionInherentKeepAliveFeature>(this);
         }
 
         // Used by unit tests
@@ -347,11 +351,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
                 throw;
             }
 
-            if (transportType == HttpTransportType.LongPolling)
+            // Disable keep alives for long polling
-            {
+            _hasInherentKeepAlive = transportType == HttpTransportType.LongPolling;
-                // Disable keep alives for long polling
-                Features.Set<IConnectionInherentKeepAliveFeature>(new ConnectionInherentKeepAliveFeature(_httpClient.Timeout));
-            }
 
             // We successfully started, set the transport properties (we don't want to set these until the transport is definitely running).
             _transport = transport;
@@ -465,4 +466,4 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
             return negotiationResponse;
         }
     }
-}
+}

src/Microsoft.AspNetCore.Http.Connections.Common/ConnectionInherentKeepAliveFeature.cs

@@ -1,18 +0,0 @@
-// Copyright (c) .NET Foundation. All rights reserved.
-// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
-
-using System;
-using Microsoft.AspNetCore.Connections.Features;
-
-namespace Microsoft.AspNetCore.Http.Connections.Features
-{
-    public class ConnectionInherentKeepAliveFeature : IConnectionInherentKeepAliveFeature
-    {
-        public TimeSpan KeepAliveInterval { get; }
-
-        public ConnectionInherentKeepAliveFeature(TimeSpan keepAliveInterval)
-        {
-            KeepAliveInterval = keepAliveInterval;
-        }
-    }
-}

src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs

@@ -24,7 +24,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
                                          IConnectionHeartbeatFeature,
                                          ITransferFormatFeature,
                                          IHttpContextFeature,
-                                         IHttpTransportFeature
+                                         IHttpTransportFeature,
+                                         IConnectionInherentKeepAliveFeature
     {
         private readonly object _itemsLock = new object();
         private readonly object _heartbeatLock = new object();
@@ -65,6 +66,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
             Features.Set<ITransferFormatFeature>(this);
             Features.Set<IHttpContextFeature>(this);
             Features.Set<IHttpTransportFeature>(this);
+            Features.Set<IConnectionInherentKeepAliveFeature>(this);
         }
 
         public HttpConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application, ILogger logger = null)
@@ -97,6 +99,8 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
 
         public ClaimsPrincipal User { get; set; }
 
+        public bool HasInherentKeepAlive { get; set; }
+
         public override IDictionary<object, object> Items
         {
             get

src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs

@@ -366,10 +366,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
         private async Task ExecuteApplication(ConnectionDelegate connectionDelegate, HttpConnectionContext connection)
         {
             // Verify some initialization invariants
-            // We want to be positive that the IConnectionInherentKeepAliveFeature is initialized before invoking the application, if the long polling transport is in use.
             Debug.Assert(connection.TransportType != HttpTransportType.None, "Transport has not been initialized yet");
-            Debug.Assert(connection.TransportType != HttpTransportType.LongPolling ||
-                connection.Features.Get<IConnectionInherentKeepAliveFeature>() != null, "Long-polling transport is in use but IConnectionInherentKeepAliveFeature as not configured");
 
             // Jump onto the thread pool thread so blocking user code doesn't block the setup of the
             // connection and transport
@@ -555,7 +552,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Internal
             // Configure transport-specific features.
             if (transportType == HttpTransportType.LongPolling)
             {
-                connection.Features.Set<IConnectionInherentKeepAliveFeature>(new ConnectionInherentKeepAliveFeature(options.LongPolling.PollTimeout));
+                connection.HasInherentKeepAlive = true;
 
                 // For long polling, the requests come and go but the connection is still alive.
                 // To make the IHttpContextFeature work well, we make a copy of the relevant properties

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

@@ -720,7 +720,9 @@ namespace Microsoft.AspNetCore.SignalR.Client
         {
             // Check if we need keep-alive
             Timer timeoutTimer = null;
-            if (connectionState.Connection.Features.Get<IConnectionInherentKeepAliveFeature>() == null)
+
+            // We use '!== true' because it could be null, which we treat as false.
+            if (connectionState.Connection.Features.Get<IConnectionInherentKeepAliveFeature>()?.HasInherentKeepAlive != true)
             {
                 Log.StartingServerTimeoutTimer(_logger, ServerTimeout);
                 timeoutTimer = new Timer(

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

@@ -339,7 +339,8 @@ namespace Microsoft.AspNetCore.SignalR
 
                                     UserIdentifier = userIdProvider.GetUserId(this);
 
-                                    if (Features.Get<IConnectionInherentKeepAliveFeature>() == null)
+                                    // != true needed because it could be null (which we treat as false)
+                                    if (Features.Get<IConnectionInherentKeepAliveFeature>()?.HasInherentKeepAlive != true)
                                     {
                                         // Only register KeepAlive after protocol handshake otherwise KeepAliveTick could try to write without having a ProtocolReaderWriter
                                         Features.Get<IConnectionHeartbeatFeature>()?.OnHeartbeat(state => ((HubConnectionContext)state).KeepAliveTick(), this);

test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionDispatcherTests.cs

@@ -1587,12 +1587,12 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests
                 var options = new HttpConnectionDispatcherOptions();
                 options.LongPolling.PollTimeout = TimeSpan.FromMilliseconds(1); // We don't care about the poll itself
 
-                Assert.Null(connection.Features.Get<IConnectionInherentKeepAliveFeature>());
-
                 await dispatcher.ExecuteAsync(context, options, app).OrTimeout();
 
-                Assert.NotNull(connection.Features.Get<IConnectionInherentKeepAliveFeature>());
+                Assert.True(connection.HasInherentKeepAlive);
-                Assert.Equal(options.LongPolling.PollTimeout, connection.Features.Get<IConnectionInherentKeepAliveFeature>().KeepAliveInterval);
+
+                // Check via the feature as well to make sure it's there.
+                Assert.True(connection.Features.Get<IConnectionInherentKeepAliveFeature>().HasInherentKeepAlive);
             }
         }
 

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Transport.cs

@@ -9,6 +9,7 @@ using System.Reflection;
 using System.Text;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
+using Microsoft.AspNetCore.Connections.Features;
 using Microsoft.AspNetCore.Http.Connections;
 using Microsoft.AspNetCore.Http.Connections.Client;
 using Microsoft.AspNetCore.Http.Connections.Client.Internal;
@@ -69,6 +70,35 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
                 Assert.True(requestsExecuted);
             }
 
+            [Theory]
+            [InlineData(HttpTransportType.LongPolling, true)]
+            [InlineData(HttpTransportType.ServerSentEvents, false)]
+            public async Task HttpConnectionSetsInherentKeepAliveFeature(HttpTransportType transportType, bool expectedValue)
+            {
+                var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);
+
+                testHttpHandler.OnRequest((request, next, token) =>
+                {
+                    return Task.FromResult(ResponseUtils.CreateResponse(HttpStatusCode.NoContent));
+                });
+
+                testHttpHandler.OnNegotiate((_, cancellationToken) =>
+                {
+                    return ResponseUtils.CreateResponse(HttpStatusCode.OK, ResponseUtils.CreateNegotiationContent());
+                });
+
+                await WithConnectionAsync(
+                    CreateConnection(testHttpHandler, transportType: transportType),
+                    async (connection) =>
+                    {
+                        await connection.StartAsync(TransferFormat.Text).OrTimeout();
+
+                        var feature = connection.Features.Get<IConnectionInherentKeepAliveFeature>();
+                        Assert.NotNull(feature);
+                        Assert.Equal(expectedValue, feature.HasInherentKeepAlive);
+                    });
+            }
+
             [Theory]
             [InlineData(HttpTransportType.LongPolling)]
             [InlineData(HttpTransportType.ServerSentEvents)]