Skip to content

Commit deebaea

Browse files
MihaZupanMihaZupan
authored
[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
1 parent 0f6284e commit deebaea

File tree

4 files changed

+197
-4
lines changed

4 files changed

+197
-4
lines changed

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

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase
6262
private int _readLength;
6363

6464
private long _idleSinceTickCount;
65+
private int _keepAliveTimeoutSeconds; // 0 == no timeout
6566
private bool _inUse;
6667
private bool _detachedFromPool;
6768
private bool _canRetry;
@@ -145,9 +146,14 @@ private void Dispose(bool disposing)
145146

146147
/// <summary>Prepare an idle connection to be used for a new request.</summary>
147148
/// <param name="async">Indicates whether the coming request will be sync or async.</param>
148-
/// <returns>True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.</returns>
149+
/// <returns>True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data.</returns>
149150
public bool PrepareForReuse(bool async)
150151
{
152+
if (CheckKeepAliveTimeoutExceeded())
153+
{
154+
return false;
155+
}
156+
151157
// We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
152158
// If the read-ahead task is completed, then we've received either EOF or erroneous data the connection, so it's not usable.
153159
if (_readAheadTask is not null)
@@ -193,9 +199,14 @@ public bool PrepareForReuse(bool async)
193199
}
194200

195201
/// <summary>Check whether a currently idle connection is still usable, or should be scavenged.</summary>
196-
/// <returns>True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.</returns>
202+
/// <returns>True if connection can be used, false if it is invalid due to a timeout or receiving EOF or unexpected data.</returns>
197203
public override bool CheckUsabilityOnScavenge()
198204
{
205+
if (CheckKeepAliveTimeoutExceeded())
206+
{
207+
return false;
208+
}
209+
199210
// We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
200211
if (_readAheadTask is null)
201212
{
@@ -223,6 +234,15 @@ async ValueTask<int> ReadAheadWithZeroByteReadAsync()
223234
}
224235
}
225236

237+
private bool CheckKeepAliveTimeoutExceeded()
238+
{
239+
// We intentionally honor the Keep-Alive timeout on all HTTP/1.X versions, not just 1.0. This is to maximize compat with
240+
// servers that use a lower idle timeout than the client, but give us a hint in the form of a Keep-Alive timeout parameter.
241+
// If _keepAliveTimeoutSeconds is 0, no timeout has been set.
242+
return _keepAliveTimeoutSeconds != 0 &&
243+
GetIdleTicks(Environment.TickCount64) >= _keepAliveTimeoutSeconds * 1000;
244+
}
245+
226246
private ValueTask<int>? ConsumeReadAheadTask()
227247
{
228248
if (Interlocked.CompareExchange(ref _readAheadTaskLock, 1, 0) == 0)
@@ -1103,14 +1123,62 @@ private static void ParseHeaderNameValue(HttpConnection connection, ReadOnlySpan
11031123
}
11041124
else
11051125
{
1106-
// Request headers returned on the response must be treated as custom headers.
11071126
string headerValue = connection.GetResponseHeaderValueWithCaching(descriptor, value, valueEncoding);
1127+
1128+
if (ReferenceEquals(descriptor.KnownHeader, KnownHeaders.KeepAlive))
1129+
{
1130+
// We are intentionally going against RFC to honor the Keep-Alive header even if
1131+
// we haven't received a Keep-Alive connection token to maximize compat with servers.
1132+
connection.ProcessKeepAliveHeader(headerValue);
1133+
}
1134+
1135+
// Request headers returned on the response must be treated as custom headers.
11081136
response.Headers.TryAddWithoutValidation(
11091137
(descriptor.HeaderType & HttpHeaderType.Request) == HttpHeaderType.Request ? descriptor.AsCustomHeader() : descriptor,
11101138
headerValue);
11111139
}
11121140
}
11131141

1142+
private void ProcessKeepAliveHeader(string keepAlive)
1143+
{
1144+
var parsedValues = new ObjectCollection<NameValueHeaderValue>();
1145+
1146+
if (NameValueHeaderValue.GetNameValueListLength(keepAlive, 0, ',', parsedValues) == keepAlive.Length)
1147+
{
1148+
foreach (NameValueHeaderValue nameValue in parsedValues)
1149+
{
1150+
// 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.
1151+
if (string.Equals(nameValue.Name, "timeout", StringComparison.OrdinalIgnoreCase))
1152+
{
1153+
if (!string.IsNullOrEmpty(nameValue.Value) &&
1154+
HeaderUtilities.TryParseInt32(nameValue.Value, out int timeout) &&
1155+
timeout >= 0)
1156+
{
1157+
// Some servers are very strict with closing the connection exactly at the timeout.
1158+
// Avoid using the connection if it is about to exceed the timeout to avoid resulting request failures.
1159+
const int OffsetSeconds = 1;
1160+
1161+
if (timeout <= OffsetSeconds)
1162+
{
1163+
_connectionClose = true;
1164+
}
1165+
else
1166+
{
1167+
_keepAliveTimeoutSeconds = timeout - OffsetSeconds;
1168+
}
1169+
}
1170+
}
1171+
else if (string.Equals(nameValue.Name, "max", StringComparison.OrdinalIgnoreCase))
1172+
{
1173+
if (nameValue.Value == "0")
1174+
{
1175+
_connectionClose = true;
1176+
}
1177+
}
1178+
}
1179+
}
1180+
}
1181+
11141182
private void WriteToBuffer(ReadOnlySpan<byte> source)
11151183
{
11161184
Debug.Assert(source.Length <= _writeBuffer.Length - _writeOffset);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2073,7 +2073,7 @@ static bool IsUsableConnection(HttpConnectionBase connection, long nowTicks, Tim
20732073

20742074
if (!connection.CheckUsabilityOnScavenge())
20752075
{
2076-
if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Unexpected data or EOF received.");
2076+
if (NetEventSource.Log.IsEnabled()) connection.Trace($"Scavenging connection. Keep-Alive timeout exceeded, unexpected data or EOF received.");
20772077
return false;
20782078
}
20792079

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

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Net.Test.Common;
5+
using System.Threading.Tasks;
6+
using Xunit;
7+
using Xunit.Abstractions;
8+
9+
namespace System.Net.Http.Functional.Tests
10+
{
11+
[ConditionalClass(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))]
12+
public sealed class SocketsHttpHandler_Http1KeepAlive_Test : HttpClientHandlerTestBase
13+
{
14+
public SocketsHttpHandler_Http1KeepAlive_Test(ITestOutputHelper output) : base(output) { }
15+
16+
[Fact]
17+
public async Task Http10Response_ConnectionIsReusedFor10And11()
18+
{
19+
await LoopbackServer.CreateClientAndServerAsync(async uri =>
20+
{
21+
using HttpClient client = CreateHttpClient();
22+
23+
await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version10, exactVersion: true));
24+
await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version11, exactVersion: true));
25+
await client.SendAsync(CreateRequest(HttpMethod.Get, uri, HttpVersion.Version10, exactVersion: true));
26+
},
27+
server => server.AcceptConnectionAsync(async connection =>
28+
{
29+
HttpRequestData request = await connection.ReadRequestDataAsync();
30+
Assert.Equal(0, request.Version.Minor);
31+
await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n1");
32+
connection.CompleteRequestProcessing();
33+
34+
request = await connection.ReadRequestDataAsync();
35+
Assert.Equal(1, request.Version.Minor);
36+
await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n2");
37+
connection.CompleteRequestProcessing();
38+
39+
request = await connection.ReadRequestDataAsync();
40+
Assert.Equal(0, request.Version.Minor);
41+
await connection.WriteStringAsync("HTTP/1.0 200 OK\r\nContent-Length: 1\r\n\r\n3");
42+
}));
43+
}
44+
45+
[OuterLoop("Uses Task.Delay")]
46+
[Fact]
47+
public async Task Http1ResponseWithKeepAliveTimeout_ConnectionRecycledAfterTimeout()
48+
{
49+
await LoopbackServer.CreateClientAndServerAsync(async uri =>
50+
{
51+
using HttpClient client = CreateHttpClient();
52+
53+
await client.GetAsync(uri);
54+
55+
await Task.Delay(2000);
56+
await client.GetAsync(uri);
57+
},
58+
async server =>
59+
{
60+
await server.AcceptConnectionAsync(async connection =>
61+
{
62+
await connection.ReadRequestDataAsync();
63+
await connection.WriteStringAsync("HTTP/1.1 200 OK\r\nKeep-Alive: timeout=2\r\nContent-Length: 1\r\n\r\n1");
64+
connection.CompleteRequestProcessing();
65+
66+
await Assert.ThrowsAnyAsync<Exception>(() => connection.ReadRequestDataAsync());
67+
});
68+
69+
await server.AcceptConnectionSendResponseAndCloseAsync();
70+
});
71+
}
72+
73+
[Theory]
74+
[InlineData("timeout=1000", true)]
75+
[InlineData("timeout=30", true)]
76+
[InlineData("timeout=0", false)]
77+
[InlineData("timeout=1", false)]
78+
[InlineData("foo, bar=baz, timeout=30", true)]
79+
[InlineData("foo, bar=baz, timeout=0", false)]
80+
[InlineData("timeout=-1", true)]
81+
[InlineData("timeout=abc", true)]
82+
[InlineData("max=1", true)]
83+
[InlineData("max=0", false)]
84+
[InlineData("max=-1", true)]
85+
[InlineData("max=abc", true)]
86+
[InlineData("timeout=30, max=1", true)]
87+
[InlineData("timeout=30, max=0", false)]
88+
[InlineData("timeout=0, max=1", false)]
89+
[InlineData("timeout=0, max=0", false)]
90+
public async Task Http1ResponseWithKeepAlive_ConnectionNotReusedForShortTimeoutOrMax0(string keepAlive, bool shouldReuseConnection)
91+
{
92+
await LoopbackServer.CreateClientAndServerAsync(async uri =>
93+
{
94+
using HttpClient client = CreateHttpClient();
95+
96+
await client.GetAsync(uri);
97+
await client.GetAsync(uri);
98+
},
99+
async server =>
100+
{
101+
await server.AcceptConnectionAsync(async connection =>
102+
{
103+
await connection.ReadRequestDataAsync();
104+
await connection.WriteStringAsync($"HTTP/1.{Random.Shared.Next(10)} 200 OK\r\nKeep-Alive: {keepAlive}\r\nContent-Length: 1\r\n\r\n1");
105+
connection.CompleteRequestProcessing();
106+
107+
if (shouldReuseConnection)
108+
{
109+
await connection.HandleRequestAsync();
110+
}
111+
else
112+
{
113+
await Assert.ThrowsAnyAsync<Exception>(() => connection.ReadRequestDataAsync());
114+
}
115+
});
116+
117+
if (!shouldReuseConnection)
118+
{
119+
await server.AcceptConnectionSendResponseAndCloseAsync();
120+
}
121+
});
122+
}
123+
}
124+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@
134134
Link="Common\System\Net\Http\HttpClientHandlerTest.DefaultProxyCredentials.cs" />
135135
<Compile Include="HttpClientHandlerTest.AltSvc.cs" />
136136
<Compile Include="SocketsHttpHandlerTest.Cancellation.cs" />
137+
<Compile Include="SocketsHttpHandlerTest.Http1KeepAlive.cs" />
137138
<Compile Include="SocketsHttpHandlerTest.Http2FlowControl.cs" />
138139
<Compile Include="SocketsHttpHandlerTest.Http2KeepAlivePing.cs" />
139140
<Compile Include="HttpClientHandlerTest.Connect.cs" />

0 commit comments

Comments
 (0)