Respect the Keep-Alive response header on HTTP/1.1 as well (#73585)

* Respect the Keep-Alive response header on HTTP/1.1 as well

* Add some more comments

Author: MihaZupan

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -236,7 +236,8 @@ async ValueTask<int> ReadAheadWithZeroByteReadAsync()
 
         private bool CheckKeepAliveTimeoutExceeded()
         {
-            // We only honor a Keep-Alive timeout on HTTP/1.0 responses.
+            // We intentionally honor the Keep-Alive timeout on all HTTP/1.X versions, not just 1.0. This is to maximize compat with
+            // servers that use a lower idle timeout than the client, but give us a hint in the form of a Keep-Alive timeout parameter.
             // If _keepAliveTimeoutSeconds is 0, no timeout has been set.
             return _keepAliveTimeoutSeconds != 0 &&
                 GetIdleTicks(Environment.TickCount64) >= _keepAliveTimeoutSeconds * 1000;
@@ -665,11 +666,6 @@ public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request,
                     ParseHeaderNameValue(this, line.Span, response, isFromTrailer: false);
                 }
 
-                if (response.Version.Minor == 0)
-                {
-                    ProcessHttp10KeepAliveHeader(response);
-                }
-
                 if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.ResponseHeadersStop();
 
                 if (allowExpect100ToContinue != null)
@@ -1124,49 +1120,58 @@ private static void ParseHeaderNameValue(HttpConnection connection, ReadOnlySpan
             }
             else
             {
-                // Request headers returned on the response must be treated as custom headers.
                 string headerValue = connection.GetResponseHeaderValueWithCaching(descriptor, value, valueEncoding);
+
+                if (descriptor.Equals(KnownHeaders.KeepAlive))
+                {
+                    // We are intentionally going against RFC to honor the Keep-Alive header even if
+                    // we haven't received a Keep-Alive connection token to maximize compat with servers.
+                    connection.ProcessKeepAliveHeader(headerValue);
+                }
+
+                // Request headers returned on the response must be treated as custom headers.
                 response.Headers.TryAddWithoutValidation(
                     (descriptor.HeaderType & HttpHeaderType.Request) == HttpHeaderType.Request ? descriptor.AsCustomHeader() : descriptor,
                     headerValue);
             }
         }
 
-        private void ProcessHttp10KeepAliveHeader(HttpResponseMessage response)
+        private void ProcessKeepAliveHeader(string keepAlive)
         {
-            if (response.Headers.NonValidated.TryGetValues(KnownHeaders.KeepAlive.Name, out HeaderStringValues keepAliveValues))
-            {
-                string keepAlive = keepAliveValues.ToString();
-                var parsedValues = new UnvalidatedObjectCollection<NameValueHeaderValue>();
+            var parsedValues = new UnvalidatedObjectCollection<NameValueHeaderValue>();
 
-                if (NameValueHeaderValue.GetNameValueListLength(keepAlive, 0, ',', parsedValues) == keepAlive.Length)
+            if (NameValueHeaderValue.GetNameValueListLength(keepAlive, 0, ',', parsedValues) == keepAlive.Length)
+            {
+                foreach (NameValueHeaderValue nameValue in parsedValues)
                 {
-                    foreach (NameValueHeaderValue nameValue in parsedValues)
+                    // The HTTP/1.1 spec does not define any parameters for the Keep-Alive header, so we are using the de facto standard ones - timeout and max.
+                    if (string.Equals(nameValue.Name, "timeout", StringComparison.OrdinalIgnoreCase))
                     {
-                        if (string.Equals(nameValue.Name, "timeout", StringComparison.OrdinalIgnoreCase))
+                        if (!string.IsNullOrEmpty(nameValue.Value) &&
+                            HeaderUtilities.TryParseInt32(nameValue.Value, out int timeout) &&
+                            timeout >= 0)
                         {
-                            if (!string.IsNullOrEmpty(nameValue.Value) &&
-                                HeaderUtilities.TryParseInt32(nameValue.Value, out int timeout) &&
-                                timeout >= 0)
+                            // Some servers are very strict with closing the connection exactly at the timeout.
+                            // Avoid using the connection if it is about to exceed the timeout to avoid resulting request failures.
+                            const int OffsetSeconds = 1;
+
+                            if (timeout <= OffsetSeconds)
                             {
-                                if (timeout == 0)
-                                {
-                                    _connectionClose = true;
-                                }
-                                else
-                                {
-                                    _keepAliveTimeoutSeconds = timeout;
-                                }
+                                _connectionClose = true;
                             }
-                        }
-                        else if (string.Equals(nameValue.Name, "max", StringComparison.OrdinalIgnoreCase))
-                        {
-                            if (nameValue.Value == "0")
+                            else
                             {
-                                _connectionClose = true;
+                                _keepAliveTimeoutSeconds = timeout - OffsetSeconds;
                             }
                         }
                     }
+                    else if (string.Equals(nameValue.Name, "max", StringComparison.OrdinalIgnoreCase))
+                    {
+                        if (nameValue.Value == "0")
+                        {
+                            _connectionClose = true;
+                        }
+                    }
                 }
             }
         }

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs

@@ -44,7 +44,7 @@ await LoopbackServer.CreateClientAndServerAsync(async uri =>
 
         [OuterLoop("Uses Task.Delay")]
         [Fact]
-        public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout()
+        public async Task Http1ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout()
         {
             await LoopbackServer.CreateClientAndServerAsync(async uri =>
             {
@@ -60,7 +60,7 @@ await LoopbackServer.CreateClientAndServerAsync(async uri =>
                 await server.AcceptConnectionAsync(async connection =>
                 {
                     await connection.ReadRequestDataAsync();
-                    await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nKeep-Alive: timeout=1\r\nContent-Length: 1\r\n\r\n1");
+                    await connection.WriteStringAsync("HTTP/1.1 200 OK\r\nKeep-Alive: timeout=2\r\nContent-Length: 1\r\n\r\n1");
                     connection.CompleteRequestProcessing();
 
                     await Assert.ThrowsAnyAsync<Exception>(() => connection.ReadRequestDataAsync());
@@ -74,6 +74,7 @@ await server.AcceptConnectionAsync(async connection =>
         [InlineData("timeout=1000", true)]
         [InlineData("timeout=30", true)]
         [InlineData("timeout=0", false)]
+        [InlineData("timeout=1", false)]
         [InlineData("foo, bar=baz, timeout=30", true)]
         [InlineData("foo, bar=baz, timeout=0", false)]
         [InlineData("timeout=-1", true)]
@@ -86,7 +87,7 @@ await server.AcceptConnectionAsync(async connection =>
         [InlineData("timeout=30, max=0", false)]
         [InlineData("timeout=0, max=1", false)]
         [InlineData("timeout=0, max=0", false)]
-        public async Task Http10ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection)
+        public async Task Http1ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection)
         {
             await LoopbackServer.CreateClientAndServerAsync(async uri =>
             {
@@ -100,7 +101,7 @@ await LoopbackServer.CreateClientAndServerAsync(async uri =>
                 await server.AcceptConnectionAsync(async connection =>
                 {
                     await connection.ReadRequestDataAsync();
-                    await connection.WriteStringAsync($"HTTP/1.0 200 OK\r\nKeep-Alive: {keepAlive}\r\nContent-Length: 1\r\n\r\n1");
+                    await connection.WriteStringAsync($"HTTP/1.{Random.Shared.Next(10)} 200 OK\r\nKeep-Alive: {keepAlive}\r\nContent-Length: 1\r\n\r\n1");
                     connection.CompleteRequestProcessing();
 
                     if (shouldReuseConnection)
@@ -119,32 +120,5 @@ await server.AcceptConnectionAsync(async connection =>
                 }
             });
         }
-
-        [Theory]
-        [InlineData("timeout=1")]
-        [InlineData("timeout=0")]
-        [InlineData("max=1")]
-        [InlineData("max=0")]
-        public async Task Http11ResponseWithKeepAlive_KeepAliveIsIgnored(string keepAlive)
-        {
-            await LoopbackServer.CreateClientAndServerAsync(async uri =>
-            {
-                using HttpClient client = CreateHttpClient();
-
-                await client.GetAsync(uri);
-                await client.GetAsync(uri);
-            },
-            async server =>
-            {
-                await server.AcceptConnectionAsync(async connection =>
-                {
-                    await connection.ReadRequestDataAsync();
-                    await connection.WriteStringAsync($"HTTP/1.1 200 OK\r\nKeep-Alive: {keepAlive}\r\nContent-Length: 1\r\n\r\n1");
-                    connection.CompleteRequestProcessing();
-
-                    await connection.HandleRequestAsync();
-                });
-            });
-        }
     }
 }