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.

Sign up for GitHub

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.1 as well #73585

Merged
merged 2 commits into from
Aug 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abd649d
Respect the Keep-Alive response header on HTTP/1.1 as well
MihaZupan Aug 8, 2022
9a1e83b
Add some more comments
MihaZupan Aug 12, 2022
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
67 changes: 36 additions & 31 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,8 @@ async ValueTask<int> ReadAheadWithZeroByteReadAsync()

private bool CheckKeepAliveTimeoutExceeded()
{
// We only honor a Keep-Alive timeout on HTTP/1.0 responses.
MihaZupan marked this conversation as resolved.
Show resolved Hide resolved
// 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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
}
}
}
}
Expand Down
36 changes: 5 additions & 31 deletions src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http1KeepAlive.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
{
Expand All @@ -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());
Expand All @@ -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)]
Expand All @@ -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 =>
{
Expand All @@ -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)
Expand All @@ -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();
});
});
}
}
}