[8.0] Optionally respect HTTP/1.0 keep-Alive for HTTP.sys #57182
Conversation
adityamandaleeka
requested review from
BrennanConroy,
JamesNK,
halter73 and
mgravell
as code owners
ghost
added
the
area-networking
| public async Task ResponseHeaders_HTTP10KeepAliveRequest_Gets11Close() | ||
| public async Task ResponseHeaders_HTTP10KeepAliveRequest_KeepAliveHeader_RespectsSwitch() | ||
| { | ||
| AppContext.SetSwitch("Microsoft.AspNetCore.Server.HttpSys.RespectHttp10KeepAlive", true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
See KestrelServerOptions.FinOnError.
There was a problem hiding this comment.
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
| private string? _requestQueueName; | ||
|
|
||
| private const string RespectHttp10KeepAliveSwitch = "Microsoft.AspNetCore.Server.HttpSys.RespectHttp10KeepAlive"; | ||
| internal bool RespectHttp10KeepAlive = AppContext.TryGetSwitch(RespectHttp10KeepAliveSwitch, out var enabled) && enabled; |
There was a problem hiding this comment.
I might say "// Internal for testing".
adityamandaleeka
added
the
Servicing-consider
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the review!
adityamandaleeka
added
Servicing-approved
7 participants
Optionally respect HTTP/1.0 keep-Alive for HTTP.sys
#56558 was done in 9.0 and made it so we respect keep-alives from 1.0 clients in HTTP.sys. This effectively backports that logic to 8.0 but in an opt-in way (behind an AppContext switch).
Description
This change adds an AppContext switch that, when enabled (it's off by default), will allow our HTTP.sys server to respect HTTP/1.0 clients'
Connection: Keep-Aliveheader.Fixes #56223
Customer Impact
With this change, servers that serve HTTP/1.0 clients can opt in to respecting the keep-alive header, and thereby save on the fixed costs of connection set up/teardown. For situations where there is a significant amount of 1.0 traffic, this can lead to significant cost savings.
Regression?
Risk
This behavior is opt-in behind an AppContext switch.
Verification
Packaging changes reviewed?