SocketsHttpHandler should proactively handle EOF from server on idle HTTP/1.1 connections

[geoffkizer]

From RFC 2616:

   When a client or server wishes to time-out it SHOULD issue a graceful
   close on the transport connection. Clients and servers SHOULD both
   constantly watch for the other side of the transport close, and
   respond to it as appropriate. If a client or server does not detect
   the other side's close promptly it could cause unnecessary resource
   drain on the network.

SocketsHttpHandler does not do this currently. We do issue a "read-ahead" read on idle connections, and we do close the connection during scavenging if we detect (via the read-ahead) that the server has closed the connection. But scavenging only happens periodically.

We could instead close the connection immediately when the read-ahead completes (and the connection is not in use). This seems more in line with the spirit of the RFC above.

[ghost]

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

Issue Details

---

From RFC 2616:

   When a client or server wishes to time-out it SHOULD issue a graceful
   close on the transport connection. Clients and servers SHOULD both
   constantly watch for the other side of the transport close, and
   respond to it as appropriate. If a client or server does not detect
   the other side's close promptly it could cause unnecessary resource
   drain on the network.

SocketsHttpHandler does not do this currently. We do issue a "read-ahead" read on idle connections, and we do close the connection during scavenging if we detect (via the read-ahead) that the server has closed the connection. But scavenging only happens periodically.

We could instead close the connection immediately when the read-ahead completes. This seems more in line with the spirit of the RFC above.

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

[stephentoub]

We could instead close the connection immediately when the read-ahead completes (and the connection is not in use).

We used to not do this as the read ahead read valuetask was directly consumed by the http receive code, it could only have a single consumer, and we didn't want the overhead of creating multiple additional tasks and continuations to implement this. But now that we structured the read differently, already paying an extra state machine to be able to do a zero-byte read, it should be trivial to add this logic into ReadAheadWithZeroByteReadAsync, just checking the actual number of bytes read from the second actual read before returning it.

[geoffkizer]

Aside from the benefits stated above -- that is, being more RFC compliant and playing nicer with servers -- this would also allow us to close these dead connections more promptly, which would reduce resource utilization overall (including the kernel resources for the socket itself). That seems like a nice benefit.

I wonder if we could/should handle connection idle timeout and lifetime in a similar way. That is, we'd managed timers for these per-connection and then if the timer expires, remove the connection from the pool.

This might add some per-connection overhead, but perhaps we could make it small in practice. For example, defer timer creation until it's actually needed, share a timer for both idle and lifetime (we only really need one for whichever will expire first), even pool timers if we need to.

This would have a couple advantages:

(1) Get rid of (most of) the scavenging logic... we would still need to clean up pools themselves when they become unused, but this is a small fraction of the current logic, and doesn't need to happen frequently. This would also help reduce contention on the connection pool lock.
(2) Promptly clean up connections that are no longer valid, which reduces both local resource usage as well as being nicer for servers.
(3) Remove the need to proactively check the validity of a connection when retrieving it from the pool. This is a small cost, so probably doesn't matter much from a perf point of view... but it certainly wouldn't hurt. And it's a nice simplification in the code, since there is some complexity associated with dealing with invalid connections and having to loop and find another connection.

Additionally, for idle timeout specifically: Currently we don't check idle timeout when we retrieve an idle connection from the pool; we only check during scavenging. This means our idle timeout is not very exact in practice; the scavenge period is 1/4 of the idle timeout period, so the "effective" idle timeout is actually somewhere between the 1 to 1.25 times the specified idle timeout. This may be surprising to customers, especially if they are trying to use the idle timeout to avoid issues that can occur when the server decides to idle timeout the connection before the client does.

(We could of course enforce idle timeout strictly when we retrieve an idle connection from the pool -- in fact we used to do this, but removed it. But the above approach would provide the same behavior as well as the other benefits above.)

The implementation details here would be a bit different for HTTP/1.1 and HTTP2, but they could probably share some logic.

And for better or worse, we have not yet implemented scavenging for HTTP3 connections. See #54968.

[MihaZupan]

For anyone working on this in the future, this is the sort of test we may expect to pass with this change: https://gist.github.com/MihaZupan/2e616b1378f31560a986b42ee9d98ce5