From 4f1c56fd89f72495899fc2c076f37ea75f158ce7 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Fri, 12 Aug 2022 20:35:06 +0200 Subject: [PATCH] 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 --- .../Http/SocketsHttpHandler/HttpConnection.cs | 67 ++++++++++--------- .../SocketsHttpHandlerTest.Http1KeepAlive.cs | 36 ++-------- 2 files changed, 41 insertions(+), 62 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index 8d55e073efe..550d3e3ba81 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -236,7 +236,8 @@ async ValueTask 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 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(); + var parsedValues = new UnvalidatedObjectCollection(); - 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; + } + } } } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs index 75ed4219e17..db03b063d17 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs @@ -44,7 +44,7 @@ public async Task Http10Response_ConnectionIsReusedFor10And11() [OuterLoop("Uses Task.Delay")] [Fact] - public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout() + public async Task Http1ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout() { await LoopbackServer.CreateClientAndServerAsync(async uri => { @@ -60,7 +60,7 @@ public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTime 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(() => connection.ReadRequestDataAsync()); @@ -74,6 +74,7 @@ public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTime [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 @@ public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTime [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 @@ public async Task Http10ResponseWithKeepAlive_ConnectionNotReusedForShortTimeout 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 @@ public async Task Http10ResponseWithKeepAlive_ConnectionNotReusedForShortTimeout } }); } - - [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(); - }); - }); - } } } -- GitLab