SocketsHttpHandler does not respect the Keep-Alive header

[MihaZupan]

RFC 2068 defines a Keep-Alive header that the server can send on HTTP/1.0 responses.

For example:

Keep-Alive: timeout=15, max=100
Connection: Keep-Alive

which indicates that the connection should not be reused after idling for longer than 15 seconds, or for more than 100 requests.

SocketsHttpHandler currently completely ignores this header.
As a result, the server may respond to otherwise-valid requests with errors.

We should consider honoring this header to improve compatibility with legacy servers.
Browsers apparently broadly support this as well: browser compatibility.

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

RFC 2068 defines a Keep-Alive header that the server can send on HTTP/1.0 responses.

For example:

Keep-Alive: timeout=15, max=100
Connection: Keep-Alive

which indicates that the connection should not be reused after idling for longer than 15 seconds, or for more than 100 requests.

SocketsHttpHandler currently completely ignores this header.
As a result, the server may respond to otherwise-valid requests with errors.

We should consider honoring this header to improve compatibility with legacy servers.
Browsers apparently broadly support this as well: browser compatibility.

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[samsp-msft]

Notes:

• The Keep-Alive header is only valid for Http/1.0, and is not supported in Http 1.1
• Servers may return error codes if a request is made after the timeout period. As HttpClient doesn't respect the header, but keeps the connection alive, it is creating a problem for servers that are still implemented as HTTP 1.0.
• We would need to recognize connections where the 1.0 keep-alive header is included and either:
  • seed the timeout into the connection's PooledConnectionIdleTimout value
  • close the connection as it doesn't support the semantics we expect.
• We can ignore the Max value provided we aren't doing request pipelining

[halter73]

The Keep-Alive header is only valid for Http/1.0, and is not supported in Http 1.1

Keep-Alive isn't exactly unsupported by HTTP/1.1. Keep-Alive is called out explicitly in RFC 7230 Appendix A as something clients and servers may wish to stay compatible with:

Some clients and servers might wish to be compatible with these previous approaches to persistent connections, by explicitly negotiating for them with a "Connection: keep-alive" request header field.

https://httpwg.org/specs/rfc7230.html#rfc.section.A.1.2

The appendix goes on to mention a bunch of reasons that you may not want to send a request with a Connection: keep-alive header which all basically boil down to the idea that HTTP/1.0 servers and proxies might not have an up-to-date understanding of how HTTP/1.1 keep-alive connection works, but this appendix provides no reason not to consider the timeout specified by the Keep-Alive response header.

It makes perfect sense to me that browsers broadly support this considering it makes up for a big shortcoming in HTTP/1.1 where requests unnecessarily fail because there's a race condition when the client sends a request at the same moment server closes the connection due to its keep-alive timeout. In HttpClient scenarios, it might not be easy to retry such a request because it's unclear if the server processed the request before closing the connection in this case.

Before .NET 6, both SocketsHttpHandler and Kestrel had 120s keep alive timeouts by default, but I updated these not to match to at least avoid this issue with newer versions of HttpClient and Kestrel. The timeout specified by the Keep-Alive response header solves this in a more widely compatible, more flexible way. I see no reason for SocketsHttpHandler not to use this information the same way browsers do.

• We would need to recognize connections where the 1.0 keep-alive header is included and either:
  • seed the timeout into the connection's PooledConnectionIdleTimout value

We should do that. The PooledConnectionIdleTimout should also probably be a few seconds lower to avoid any possibility of the race condition mentioned above.

• close the connection as it doesn't support the semantics we expect.

Again, it's a bit unfair to say that the connection doesn't support the semantics we expect. These responses are coming from Node.js web servers which very much do support HTTP/1.1. There's no reason SocketsHttpHandler cannot support these connections. It supports them already, so rejecting these connections would be a huge regression. However, it could support them better by taking into account the server's keep-alive timeout.

I don't think SocketsHttpHandler should even validate that Connection: keep-alive header is present before using this timeout even though the spec requires it. If the server is going to tell the client what its keep-alive timeout is, why should the client doubt its accuracy? It's useful information to have to avoid failed requests.

[CarnaViire]

Triage: the fix should be simple enough, we should fix it in 7.0