[release/6.0] Respect the Keep-Alive response header on HTTP/1.0 (#73245

)

* Respect the Keep-Alive response header on HTTP/1.0 (#73020)

* Respect the Keep-Alive response header on HTTP/1.0

* Remove TimeoutOffset

* Update Trace message

* Update tests

* Adjust test timeouts

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

* Add some more comments

* Account for HeaderDescriptor changes in 7.0

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

@@ -62,6 +62,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase
         private int _readLength;
 
         private long _idleSinceTickCount;
+        private int _keepAliveTimeoutSeconds; // 0 == no timeout
         private bool _inUse;
         private bool _detachedFromPool;
         private bool _canRetry;
@@ -145,9 +146,14 @@ private void Dispose(bool disposing)
 
         /// <summary>Prepare an idle connection to be used for a new request.</summary>
         /// <param name="async">Indicates whether the coming request will be sync or async.</param>
-        /// <returns>True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.</returns>
+        /// <returns>True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data.</returns>
         public bool PrepareForReuse(bool async)
         {
+            if (CheckKeepAliveTimeoutExceeded())
+            {
+                return false;
+            }
+
             // We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
             // If the read-ahead task is completed, then we've received either EOF or erroneous data the connection, so it's not usable.
             if (_readAheadTask is not null)
@@ -193,9 +199,14 @@ public bool PrepareForReuse(bool async)
         }
 
         /// <summary>Check whether a currently idle connection is still usable, or should be scavenged.</summary>
-        /// <returns>True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.</returns>
+        /// <returns>True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data.</returns>
         public override bool CheckUsabilityOnScavenge()
         {
+            if (CheckKeepAliveTimeoutExceeded())
+            {
+                return false;
+            }
+
             // We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
             if (_readAheadTask is null)
             {
@@ -223,6 +234,15 @@ async ValueTask<int> ReadAheadWithZeroByteReadAsync()
             }
         }
 
+        private bool CheckKeepAliveTimeoutExceeded()
+        {
+            // 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;
+        }
+
         private ValueTask<int>? ConsumeReadAheadTask()
         {
             if (Interlocked.CompareExchange(ref _readAheadTaskLock, 1, 0) == 0)
@@ -1103,14 +1123,62 @@ 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 (ReferenceEquals(descriptor.KnownHeader, 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 ProcessKeepAliveHeader(string keepAlive)
+        {
+            var parsedValues = new ObjectCollection<NameValueHeaderValue>();
+
+            if (NameValueHeaderValue.GetNameValueListLength(keepAlive, 0, ',', parsedValues) == keepAlive.Length)
+            {
+                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.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)
+                            {
+                                _connectionClose = true;
+                            }
+                            else
+                            {
+                                _keepAliveTimeoutSeconds = timeout - OffsetSeconds;
+                            }
+                        }
+                    }
+                    else if (string.Equals(nameValue.Name, "max", StringComparison.OrdinalIgnoreCase))
+                    {
+                        if (nameValue.Value == "0")
+                        {
+                            _connectionClose = true;
+                        }
+                    }
+                }
+            }
+        }
+
         private void WriteToBuffer(ReadOnlySpan<byte> source)
         {
             Debug.Assert(source.Length <= _writeBuffer.Length - _writeOffset);

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

@@ -2073,7 +2073,7 @@ static bool IsUsableConnection(HttpConnectionBase connection, long nowTicks, Tim
 
                 if (!connection.CheckUsabilityOnScavenge())
                 {
-                    if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Unexpected data or EOF received.");
+                    if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Keep-Alive timeout exceeded, unexpected data or EOF received.");
                     return false;
                 }
 

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

@@ -0,0 +1,124 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Net.Test.Common;
+using System.Threading.Tasks;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace System.Net.Http.Functional.Tests
+{
+    [ConditionalClass(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))]
+    public sealed class SocketsHttpHandler_Http1KeepAlive_Test : HttpClientHandlerTestBase
+    {
+        public SocketsHttpHandler_Http1KeepAlive_Test(ITestOutputHelper output) : base(output) { }
+
+        [Fact]
+        public async Task Http10Response_ConnectionIsReusedFor10And11()
+        {
+            await LoopbackServer.CreateClientAndServerAsync(async uri =>
+            {
+                using HttpClient client = CreateHttpClient();
+
+                await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version10, exactVersion: true));
+                await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version11, exactVersion: true));
+                await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version10, exactVersion: true));
+            },
+            server => server.AcceptConnectionAsync(async connection =>
+            {
+                HttpRequestData request = await connection.ReadRequestDataAsync();
+                Assert.Equal(0, request.Version.Minor);
+                await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n1");
+                connection.CompleteRequestProcessing();
+
+                request = await connection.ReadRequestDataAsync();
+                Assert.Equal(1, request.Version.Minor);
+                await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n2");
+                connection.CompleteRequestProcessing();
+
+                request = await connection.ReadRequestDataAsync();
+                Assert.Equal(0, request.Version.Minor);
+                await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n3");
+            }));
+        }
+
+        [OuterLoop("Uses Task.Delay")]
+        [Fact]
+        public async Task Http1ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout()
+        {
+            await LoopbackServer.CreateClientAndServerAsync(async uri =>
+            {
+                using HttpClient client = CreateHttpClient();
+
+                await client.GetAsync(uri);
+
+                await Task.Delay(2000);
+                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: timeout=2\r\nContent-Length: 1\r\n\r\n1");
+                    connection.CompleteRequestProcessing();
+
+                    await Assert.ThrowsAnyAsync<Exception>(() => connection.ReadRequestDataAsync());
+                });
+
+                await server.AcceptConnectionSendResponseAndCloseAsync();
+            });
+        }
+
+        [Theory]
+        [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)]
+        [InlineData("timeout=abc", true)]
+        [InlineData("max=1", true)]
+        [InlineData("max=0", false)]
+        [InlineData("max=-1", true)]
+        [InlineData("max=abc", true)]
+        [InlineData("timeout=30, max=1", true)]
+        [InlineData("timeout=30, max=0", false)]
+        [InlineData("timeout=0, max=1", false)]
+        [InlineData("timeout=0, max=0", false)]
+        public async Task Http1ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection)
+        {
+            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.{Random.Shared.Next(10)} 200 OK\r\nKeep-Alive: {keepAlive}\r\nContent-Length: 1\r\n\r\n1");
+                    connection.CompleteRequestProcessing();
+
+                    if (shouldReuseConnection)
+                    {
+                        await connection.HandleRequestAsync();
+                    }
+                    else
+                    {
+                        await Assert.ThrowsAnyAsync<Exception>(() => connection.ReadRequestDataAsync());
+                    }
+                });
+
+                if (!shouldReuseConnection)
+                {
+                    await server.AcceptConnectionSendResponseAndCloseAsync();
+                }
+            });
+        }
+    }
+}

src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj

@@ -134,6 +134,7 @@
              Link="Common\System\Net\Http\HttpClientHandlerTest.DefaultProxyCredentials.cs" />
     <Compile Include="HttpClientHandlerTest.AltSvc.cs" />
     <Compile Include="SocketsHttpHandlerTest.Cancellation.cs" />
+    <Compile Include="SocketsHttpHandlerTest.Http1KeepAlive.cs" />
     <Compile Include="SocketsHttpHandlerTest.Http2FlowControl.cs" />
     <Compile Include="SocketsHttpHandlerTest.Http2KeepAlivePing.cs" />
     <Compile Include="HttpClientHandlerTest.Connect.cs" />