From 2ba292211c3f602040944b4f8c468fa51d6f0046 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Thu, 28 Jul 2022 22:23:45 +0200 Subject: [PATCH 1/5] Respect the Keep-Alive response header on HTTP/1.0 --- .../Http/SocketsHttpHandler/HttpConnection.cs | 68 +++++++- .../SocketsHttpHandler/HttpConnectionPool.cs | 2 +- .../SocketsHttpHandlerTest.Http1KeepAlive.cs | 157 ++++++++++++++++++ .../System.Net.Http.Functional.Tests.csproj | 1 + 4 files changed, 225 insertions(+), 3 deletions(-) create mode 100644 src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs 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 2d92a7b74e9a1..1cfedc0e2a2dd 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 @@ -67,6 +67,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase private int _readLength; private long _idleSinceTickCount; + private int _keepAliveTimeoutSeconds; private bool _inUse; private bool _detachedFromPool; private bool _canRetry; @@ -148,9 +149,14 @@ private void Dispose(bool disposing) /// Prepare an idle connection to be used for a new request. /// Indicates whether the coming request will be sync or async. - /// True if connection can be used, false if it is invalid due to receiving EOF or unexpected data. + /// True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data. 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) @@ -196,9 +202,14 @@ public bool PrepareForReuse(bool async) } /// Check whether a currently idle connection is still usable, or should be scavenged. - /// True if connection can be used, false if it is invalid due to receiving EOF or unexpected data. + /// True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data. 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. #pragma warning disable CA2012 // we're very careful to ensure the ValueTask is only consumed once, even though it's stored into a field _readAheadTask ??= ReadAheadWithZeroByteReadAsync(); @@ -223,6 +234,15 @@ async ValueTask ReadAheadWithZeroByteReadAsync() } } + private bool CheckKeepAliveTimeoutExceeded() + { + // Avoid reusing a connection if there is less than one second remaining before the timeout. + const int TimeoutOffsetSeconds = 1; + + return _keepAliveTimeoutSeconds != 0 && + GetIdleTicks(Environment.TickCount64) >= (_keepAliveTimeoutSeconds - TimeoutOffsetSeconds) * 1000; + } + private ValueTask? ConsumeReadAheadTask() { if (Interlocked.CompareExchange(ref _readAheadTaskLock, 1, 0) == 0) @@ -646,6 +666,11 @@ 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) @@ -1108,6 +1133,45 @@ private static void ParseHeaderNameValue(HttpConnection connection, ReadOnlySpan } } + private void ProcessHttp10KeepAliveHeader(HttpResponseMessage response) + { + if (response.Headers.NonValidated.TryGetValues(KnownHeaders.KeepAlive.Name, out HeaderStringValues keepAliveValues)) + { + string keepAlive = keepAliveValues.ToString(); + var parsedValues = new UnvalidatedObjectCollection(); + + if (NameValueHeaderValue.GetNameValueListLength(keepAlive, 0, ',', parsedValues) == keepAlive.Length) + { + foreach (NameValueHeaderValue nameValue in parsedValues) + { + if (string.Equals(nameValue.Name, "timeout", StringComparison.OrdinalIgnoreCase)) + { + if (!string.IsNullOrEmpty(nameValue.Value) && + HeaderUtilities.TryParseInt32(nameValue.Value, out int timeout) && + timeout >= 0) + { + if (timeout <= 1) + { + _connectionClose = true; + } + else + { + _keepAliveTimeoutSeconds = timeout; + } + } + } + else if (string.Equals(nameValue.Name, "max", StringComparison.OrdinalIgnoreCase)) + { + if (nameValue.Value == "0") + { + _connectionClose = true; + } + } + } + } + } + } + private void WriteToBuffer(ReadOnlySpan source) { Debug.Assert(source.Length <= _writeBuffer.Length - _writeOffset); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs index ebc5c612d9e63..f165da5e36150 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs @@ -2324,7 +2324,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 or unexpected data or EOF received."); return false; } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs new file mode 100644 index 0000000000000..da23350d136ca --- /dev/null +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs @@ -0,0 +1,157 @@ +// 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 Http10ResponseWithKeepAliveTimeout_ConnectionRecycledBeforeTimeout() + { + await LoopbackServer.CreateClientAndServerAsync(async uri => + { + using HttpClient client = CreateHttpClient(); + + await client.GetAsync(uri); + + await Task.Delay(10); + await client.GetAsync(uri); + + await Task.Delay(2500); + await client.GetAsync(uri); + }, + async server => + { + await server.AcceptConnectionAsync(async connection => + { + await connection.ReadRequestDataAsync(); + await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nKeep-Alive: timeout=3\r\nContent-Length: 1\r\n\r\n1"); + connection.CompleteRequestProcessing(); + + await connection.HandleRequestAsync(); + connection.CompleteRequestProcessing(); + + await Assert.ThrowsAnyAsync(() => connection.ReadRequestDataAsync()); + }); + + await server.AcceptConnectionSendResponseAndCloseAsync(); + }); + } + + [Theory] + [InlineData("timeout=1000", true)] + [InlineData("timeout=2", true)] + [InlineData("timeout=1", false)] + [InlineData("timeout=0", false)] + [InlineData("foo, bar=baz, timeout=2", true)] + [InlineData("foo, bar=baz, timeout=1", 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=5, max=1", true)] + [InlineData("timeout=5, max=0", false)] + [InlineData("timeout=1, max=1", false)] + [InlineData("timeout=1, max=0", false)] + public async Task Http10ResponseWithKeepAlive_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.0 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(() => connection.ReadRequestDataAsync()); + } + }); + + if (!shouldReuseConnection) + { + await server.AcceptConnectionSendResponseAndCloseAsync(); + } + }); + } + + [Theory] + [InlineData("timeout=2")] + [InlineData("timeout=1")] + [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(); + }); + }); + } + } +} diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj index 6c05b3cc0f35b..8eb14c5c99c8a 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj @@ -160,6 +160,7 @@ + From cfa2fa6e0fd30f5b690b2a5164e25310ae261f2a Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Mon, 1 Aug 2022 19:22:50 +0200 Subject: [PATCH 2/5] Remove TimeoutOffset --- .../Net/Http/SocketsHttpHandler/HttpConnection.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 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 1cfedc0e2a2dd..8d55e073efebe 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 @@ -67,7 +67,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase private int _readLength; private long _idleSinceTickCount; - private int _keepAliveTimeoutSeconds; + private int _keepAliveTimeoutSeconds; // 0 == no timeout private bool _inUse; private bool _detachedFromPool; private bool _canRetry; @@ -236,11 +236,10 @@ async ValueTask ReadAheadWithZeroByteReadAsync() private bool CheckKeepAliveTimeoutExceeded() { - // Avoid reusing a connection if there is less than one second remaining before the timeout. - const int TimeoutOffsetSeconds = 1; - + // We only honor a Keep-Alive timeout on HTTP/1.0 responses. + // If _keepAliveTimeoutSeconds is 0, no timeout has been set. return _keepAliveTimeoutSeconds != 0 && - GetIdleTicks(Environment.TickCount64) >= (_keepAliveTimeoutSeconds - TimeoutOffsetSeconds) * 1000; + GetIdleTicks(Environment.TickCount64) >= _keepAliveTimeoutSeconds * 1000; } private ValueTask? ConsumeReadAheadTask() @@ -1150,7 +1149,7 @@ private void ProcessHttp10KeepAliveHeader(HttpResponseMessage response) HeaderUtilities.TryParseInt32(nameValue.Value, out int timeout) && timeout >= 0) { - if (timeout <= 1) + if (timeout == 0) { _connectionClose = true; } From 1d41185a865f0c89fbdf64adf5257233d246ca38 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Mon, 1 Aug 2022 19:24:25 +0200 Subject: [PATCH 3/5] Update Trace message --- .../System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs index f165da5e36150..e560915288e56 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs @@ -2324,7 +2324,7 @@ static bool IsUsableConnection(HttpConnectionBase connection, long nowTicks, Tim if (!connection.CheckUsabilityOnScavenge()) { - if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Keep-Alive timeout exceeded or unexpected data or EOF received."); + if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Keep-Alive timeout exceeded, unexpected data or EOF received."); return false; } From afe5b77806e665f77634e7125712221b21d4c248 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Mon, 1 Aug 2022 19:39:47 +0200 Subject: [PATCH 4/5] Update tests --- .../SocketsHttpHandlerTest.Http1KeepAlive.cs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) 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 da23350d136ca..b08cb7fb71198 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs @@ -44,18 +44,16 @@ await LoopbackServer.CreateClientAndServerAsync(async uri => [OuterLoop("Uses Task.Delay")] [Fact] - public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledBeforeTimeout() + public async Task Http10ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout() { await LoopbackServer.CreateClientAndServerAsync(async uri => { using HttpClient client = CreateHttpClient(); await client.GetAsync(uri); - - await Task.Delay(10); await client.GetAsync(uri); - await Task.Delay(2500); + await Task.Delay(6000); await client.GetAsync(uri); }, async server => @@ -63,7 +61,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=3\r\nContent-Length: 1\r\n\r\n1"); + await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nKeep-Alive: timeout=5\r\nContent-Length: 1\r\n\r\n1"); connection.CompleteRequestProcessing(); await connection.HandleRequestAsync(); @@ -78,11 +76,10 @@ await server.AcceptConnectionAsync(async connection => [Theory] [InlineData("timeout=1000", true)] - [InlineData("timeout=2", true)] - [InlineData("timeout=1", false)] + [InlineData("timeout=5", true)] [InlineData("timeout=0", false)] - [InlineData("foo, bar=baz, timeout=2", true)] - [InlineData("foo, bar=baz, timeout=1", false)] + [InlineData("foo, bar=baz, timeout=5", true)] + [InlineData("foo, bar=baz, timeout=0", false)] [InlineData("timeout=-1", true)] [InlineData("timeout=abc", true)] [InlineData("max=1", true)] @@ -91,8 +88,8 @@ await server.AcceptConnectionAsync(async connection => [InlineData("max=abc", true)] [InlineData("timeout=5, max=1", true)] [InlineData("timeout=5, max=0", false)] - [InlineData("timeout=1, max=1", false)] - [InlineData("timeout=1, max=0", false)] + [InlineData("timeout=0, max=1", false)] + [InlineData("timeout=0, max=0", false)] public async Task Http10ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection) { await LoopbackServer.CreateClientAndServerAsync(async uri => @@ -128,8 +125,8 @@ await server.AcceptConnectionAsync(async connection => } [Theory] - [InlineData("timeout=2")] [InlineData("timeout=1")] + [InlineData("timeout=0")] [InlineData("max=1")] [InlineData("max=0")] public async Task Http11ResponseWithKeepAlive_KeepAliveIsIgnored(string keepAlive) From f4f44c6dbe42ab3297c4c7cbbb7c2d81144b914a Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Tue, 2 Aug 2022 14:39:18 +0200 Subject: [PATCH 5/5] Adjust test timeouts --- .../SocketsHttpHandlerTest.Http1KeepAlive.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) 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 b08cb7fb71198..75ed4219e1783 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs @@ -50,10 +50,9 @@ await LoopbackServer.CreateClientAndServerAsync(async uri => { using HttpClient client = CreateHttpClient(); - await client.GetAsync(uri); await client.GetAsync(uri); - await Task.Delay(6000); + await Task.Delay(2000); await client.GetAsync(uri); }, async server => @@ -61,10 +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=5\r\nContent-Length: 1\r\n\r\n1"); - connection.CompleteRequestProcessing(); - - await connection.HandleRequestAsync(); + await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nKeep-Alive: timeout=1\r\nContent-Length: 1\r\n\r\n1"); connection.CompleteRequestProcessing(); await Assert.ThrowsAnyAsync(() => connection.ReadRequestDataAsync()); @@ -76,9 +72,9 @@ await server.AcceptConnectionAsync(async connection => [Theory] [InlineData("timeout=1000", true)] - [InlineData("timeout=5", true)] + [InlineData("timeout=30", true)] [InlineData("timeout=0", false)] - [InlineData("foo, bar=baz, timeout=5", true)] + [InlineData("foo, bar=baz, timeout=30", true)] [InlineData("foo, bar=baz, timeout=0", false)] [InlineData("timeout=-1", true)] [InlineData("timeout=abc", true)] @@ -86,8 +82,8 @@ await server.AcceptConnectionAsync(async connection => [InlineData("max=0", false)] [InlineData("max=-1", true)] [InlineData("max=abc", true)] - [InlineData("timeout=5, max=1", true)] - [InlineData("timeout=5, max=0", false)] + [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 Http10ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection)