Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Aug 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,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;
Expand Down Expand Up @@ -148,9 +149,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)
Expand Down Expand Up @@ -196,9 +202,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.
#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();
Expand All @@ -223,6 +234,14 @@ async ValueTask<int> ReadAheadWithZeroByteReadAsync()
}
}

private bool CheckKeepAliveTimeoutExceeded()
{
// We only honor a Keep-Alive timeout on HTTP/1.0 responses.
// If _keepAliveTimeoutSeconds is 0, no timeout has been set.
return _keepAliveTimeoutSeconds != 0 &&
MihaZupan marked this conversation as resolved.
Show resolved Hide resolved
GetIdleTicks(Environment.TickCount64) >= _keepAliveTimeoutSeconds * 1000;
}

private ValueTask<int>? ConsumeReadAheadTask()
{
if (Interlocked.CompareExchange(ref _readAheadTaskLock, 1, 0) == 0)
Expand Down Expand Up @@ -646,6 +665,11 @@ 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)
Expand Down Expand Up @@ -1108,6 +1132,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<NameValueHeaderValue>();

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 == 0)
{
_connectionClose = true;
MihaZupan marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
_keepAliveTimeoutSeconds = timeout;
}
}
}
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, unexpected data or EOF received.");
return false;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// 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_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.0 200 OK\r\nKeep-Alive: timeout=1\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("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)]
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
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<Exception>(() => connection.ReadRequestDataAsync());
}
});

if (!shouldReuseConnection)
{
await server.AcceptConnectionSendResponseAndCloseAsync();
}
});
}

[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();
});
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@
<Compile Include="HttpClientHandlerTest.AltSvc.cs" />
<Compile Include="SocketsHttpHandlerTest.Cancellation.cs" />
<Compile Include="SocketsHttpHandlerTest.Cancellation.NonParallel.cs" />
<Compile Include="SocketsHttpHandlerTest.Http1KeepAlive.cs" />
<Compile Include="SocketsHttpHandlerTest.Http2FlowControl.cs" />
<Compile Include="SocketsHttpHandlerTest.Http2KeepAlivePing.cs" />
<Compile Include="HttpClientHandlerTest.Connect.cs" />
Expand Down