Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
19 changes: 18 additions & 1 deletion src/Servers/HttpSys/src/RequestProcessing/Response.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ internal sealed class Response
private HttpApiTypes.HTTP_RESPONSE_V2 _nativeResponse;
private HeaderCollection? _trailers;

private const string RespectHttp10KeepAliveSwitch = "Microsoft.AspNetCore.Server.HttpSys.RespectHttp10KeepAlive";
private readonly bool _respectHttp10KeepAlive = AppContext.TryGetSwitch(RespectHttp10KeepAliveSwitch, out var enabled) && enabled;

internal Response(RequestContext requestContext)
{
// TODO: Verbose log
Expand Down Expand Up @@ -390,6 +393,7 @@ internal HttpApiTypes.HTTP_FLAGS ComputeHeaders(long writeCount, bool endOfReque
var requestConnectionString = Request.Headers[HeaderNames.Connection];
var isHeadRequest = Request.IsHeadMethod;
var requestCloseSet = Matches(Constants.Close, requestConnectionString);
var requestConnectionKeepAliveSet = Matches(Constants.KeepAlive, requestConnectionString);

// Gather everything the app may have set on the response:
// Http.Sys does not allow us to specify the response protocol version, assume this is a HTTP/1.1 response when making decisions.
Expand All @@ -402,12 +406,25 @@ internal HttpApiTypes.HTTP_FLAGS ComputeHeaders(long writeCount, bool endOfReque

// Determine if the connection will be kept alive or closed.
var keepConnectionAlive = true;
if (requestVersion <= Constants.V1_0 // Http.Sys does not support "Keep-Alive: true" or "Connection: Keep-Alive"

if (requestVersion < Constants.V1_0
|| (requestVersion == Constants.V1_1 && requestCloseSet)
|| responseCloseSet)
{
keepConnectionAlive = false;
}
else if (requestVersion == Constants.V1_0)
{
// In .NET 9, we updated the behavior for 1.0 clients here to match
// RFC 2068. The new behavior is available down-level behind an
// AppContext switch.

// An HTTP/1.1 server may also establish persistent connections with
// HTTP/1.0 clients upon receipt of a Keep-Alive connection token.
// However, a persistent connection with an HTTP/1.0 client cannot make
// use of the chunked transfer-coding. From: https://www.rfc-editor.org/rfc/rfc2068#section-19.7.1
keepConnectionAlive = _respectHttp10KeepAlive && requestConnectionKeepAliveSet && !responseChunkedSet;
Copy link
Member

@Tratcher Tratcher Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responseChunkedSet is only for cases where the application sets the chunked header and does the chunking themselves, it doesn't cover the default case where the server does chunking automatically. See _boundaryType = BoundaryType.Chunked; vs _boundaryType = BoundaryType.PassThrough; below.

edit nevermind, the automatic case only happens for 1.1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

}

// Determine the body format. If the user asks to do something, let them, otherwise choose a good default for the scenario.
if (responseContentLength.HasValue)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Testing;
Expand Down Expand Up @@ -207,20 +208,85 @@ public async Task ResponseHeaders_11HeadRequestStatusCodeWithoutBody_NoContentLe
}

[ConditionalFact]
public async Task ResponseHeaders_HTTP10KeepAliveRequest_Gets11Close()
public async Task ResponseHeaders_HTTP10KeepAliveRequest_KeepAliveHeader_RespectsSwitch()
{
AppContext.SetSwitch("Microsoft.AspNetCore.Server.HttpSys.RespectHttp10KeepAlive", true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there being some complexity here around interactions between different tests. That is, if tests are running in parallel, I don't think we want this changing states. I'm pretty sure Chris suggested I create an internal property populated from the context switch but overridable by tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See KestrelServerOptions.FinOnError.

Copy link
Contributor

@mgravell mgravell Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for EnableKernelResponseBuffering, I made use of an internal property on HttpSysOptions, that is initialized in the .ctor via the app-context switch - that way, relevant tests can set it explicitly via the internal property rather than the app-context switch (IVTA is already in place)

but yes @amcasey you're right that changing app-context directly in tests makes for a race condition in parallel tests - and also if this one test fails and exits prematurely, it might never be set back to the false state, which could cause additional behaviour changes, make finding the real cause difficult

string address;
using (var server = Utilities.CreateHttpServer(out address))
{
// Track the number of times ConnectCallback is invoked to ensure the underlying socket wasn't closed.
int connectCallbackInvocations = 0;
var handler = new SocketsHttpHandler();
handler.ConnectCallback = (context, cancellationToken) =>
{
Interlocked.Increment(ref connectCallbackInvocations);
return ConnectCallback(context, cancellationToken);
};

using (var client = new HttpClient(handler))
{
// Send the first request
Task<HttpResponseMessage> responseTask = SendRequestAsync(address, usehttp11: false, sendKeepAlive: true, httpClient: client);
var context = await server.AcceptAsync(Utilities.DefaultTimeout).Before(responseTask);
context.Dispose();

HttpResponseMessage response = await responseTask;
response.EnsureSuccessStatusCode();
Assert.Equal(new Version(1, 1), response.Version);
Assert.Null(response.Headers.ConnectionClose);

// Send the second request
responseTask = SendRequestAsync(address, usehttp11: false, sendKeepAlive: true, httpClient: client);
context = await server.AcceptAsync(Utilities.DefaultTimeout).Before(responseTask);
context.Dispose();

response = await responseTask;
response.EnsureSuccessStatusCode();
Assert.Equal(new Version(1, 1), response.Version);
Assert.Null(response.Headers.ConnectionClose);
}

// Verify that ConnectCallback was only called once
Assert.Equal(1, connectCallbackInvocations);
}

AppContext.SetSwitch("Microsoft.AspNetCore.Server.HttpSys.RespectHttp10KeepAlive", false);
using (var server = Utilities.CreateHttpServer(out address))
{
var handler = new SocketsHttpHandler();
using (var client = new HttpClient(handler))
{
// Send the first request
Task<HttpResponseMessage> responseTask = SendRequestAsync(address, usehttp11: false, sendKeepAlive: true, httpClient: client);
var context = await server.AcceptAsync(Utilities.DefaultTimeout).Before(responseTask);
context.Dispose();

HttpResponseMessage response = await responseTask;
response.EnsureSuccessStatusCode();
Assert.Equal(new Version(1, 1), response.Version);
Assert.True(response.Headers.ConnectionClose.Value);
}
}
}

[ConditionalFact]
public async Task ResponseHeaders_HTTP10KeepAliveRequest_ChunkedTransferEncoding_Gets11Close()
{
string address;
using (var server = Utilities.CreateHttpServer(out address))
{
// Http.Sys does not support 1.0 keep-alives.
Task<HttpResponseMessage> responseTask = SendRequestAsync(address, usehttp11: false, sendKeepAlive: true);

var context = await server.AcceptAsync(Utilities.DefaultTimeout).Before(responseTask);
context.Response.Headers["Transfer-Encoding"] = new string[] { "chunked" };
var responseBytes = Encoding.ASCII.GetBytes("10\r\nManually Chunked\r\n0\r\n\r\n");
await context.Response.Body.WriteAsync(responseBytes, 0, responseBytes.Length);
context.Dispose();

HttpResponseMessage response = await responseTask;
response.EnsureSuccessStatusCode();
Assert.Equal(new Version(1, 1), response.Version);
Assert.True(response.Headers.TransferEncodingChunked.HasValue, "Chunked");
Assert.True(response.Headers.ConnectionClose.Value);
}
}
Expand Down Expand Up @@ -289,8 +355,9 @@ public async Task AddingControlCharactersToHeadersThrows(string key, string valu
}
}

private async Task<HttpResponseMessage> SendRequestAsync(string uri, bool usehttp11 = true, bool sendKeepAlive = false)
private async Task<HttpResponseMessage> SendRequestAsync(string uri, bool usehttp11 = true, bool sendKeepAlive = false, HttpClient httpClient = null)
{
httpClient ??= _client;
var request = new HttpRequestMessage(HttpMethod.Get, uri);
if (!usehttp11)
{
Expand All @@ -300,7 +367,7 @@ private async Task<HttpResponseMessage> SendRequestAsync(string uri, bool usehtt
{
request.Headers.Add("Connection", "Keep-Alive");
}
return await _client.SendAsync(request);
return await httpClient.SendAsync(request);
}

private async Task<HttpResponseMessage> SendHeadRequestAsync(string uri, bool usehttp11 = true)
Expand All @@ -312,4 +379,19 @@ private async Task<HttpResponseMessage> SendHeadRequestAsync(string uri, bool us
}
return await _client.SendAsync(request);
}

private static async ValueTask<Stream> ConnectCallback(SocketsHttpConnectionContext connectContext, CancellationToken ct)
{
var s = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
try
{
await s.ConnectAsync(connectContext.DnsEndPoint, ct);
return new NetworkStream(s, ownsSocket: true);
}
catch
{
s.Dispose();
throw;
}
}
}
1 change: 1 addition & 0 deletions src/Shared/HttpSys/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ internal static class Constants
internal const string HttpsScheme = "https";
internal const string Chunked = "chunked";
internal const string Close = "close";
internal const string KeepAlive = "keep-alive";
internal const string Zero = "0";
internal const string SchemeDelimiter = "://";
internal const string DefaultServerAddress = "http://localhost:5000";
Expand Down