HttpConnection regression around read-aheads on pooled https connections

[stephentoub]

When we take a connection out of the SocketsHttpHandler connection pool to process a request, we want to validate that it hasn't received any stray data, so we need to poll it. But we also know we're about to issue a read on it as part of handling the request. So, we just issue an async read as part of taking it out of the pool, and check whether that read completes, to avoid the extra poll sys call. However, the other time we poll connections is while they're in the pool, every once in a while checking every connection to see if it's still valid, but not removing it from the pool. Here, we don't want to do such a read-ahead, for several reasons:

1. On Windows, doing an async read with a buffer pins that buffer for the duration of the read. That means we'd end up pinning a buffer for every connection in the pool and for as long as that connection is idle in the pool, which has negative impact on GC.
2. If there's a pending read on a connection we then tear down, it results in an exception, which is both extra overhead and has caused developers to complain about unexpected unobserved task exceptions. We've fixed bugs in the past related to this.

So, when we create connections, even as we wrap the socket in layers of streams, we try to hold on to the socket so we can Poll it in the case where we want to check connection viability but without taking it out of the pool. In .NET Core 3.1, we were able to do this for most connections, both http and https, and only couldn't when using a proxy tunnel.
https://github.com/dotnet/corefx/blob/5e7fb3dbb33257d285857e3f9f654df457234173/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L646

But as part of adding (4b8c5fe#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R1180) and then reverting (3609f05#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R1271), this Socket-tracking code was (presumably accidentally) removed for .NET 5, with it now only checking for NetworkStream:
3609f05#diff-595f700a4c83b735d856fe14b5ebe8c7f7c18ae6e586adc8b398a997a0678274R82-R85
This means that whereas in .NET Core 3.1 we would only poll rather than read-ahead for https connections when periodically checking their status in the pool, now in .NET 5 we're doing read-aheads for https connections, and incurring the aforementioned problems that come with it (buffers pinned for long periods, extra exceptions when connections are torn down).

[ghost]

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

Issue Details

---

When we take a connection out of the SocketsHttpHandler connection pool to process a request, we want to validate that it hasn't received any stray data, so we need to poll it. But we also know we're about to issue a read on it as part of handling the request. So, we just issue an async read as part of taking it out of the pool, and check whether that read completes, to avoid the extra poll sys call. However, the other time we poll connections is while they're in the pool, every once in a while checking every connection to see if it's still valid, but not removing it from the pool. Here, we don't want to do such a read-ahead, for several reasons:

1. On Windows, doing an async read with a buffer pins that buffer for the duration of the read. That means we'd end up pinning a buffer for every connection in the pool and for as long as that connection is idle in the pool, which has negative impact on GC.
2. If there's a pending read on a connection we then tear down, it results in an exception, which is both extra overhead and has caused developers to complain about unexpected unobserved task exceptions. We've fixed bugs in the past related to this.

So, when we create connections, even as we wrap the socket in layers of streams, we try to hold on to the socket so we can Poll it in the case where we want to check connection viability but without taking it out of the pool. In .NET Core 3.1, we were able to do this for most connections, both http and https, and only couldn't when using a proxy tunnel.
https://github.com/dotnet/corefx/blob/5e7fb3dbb33257d285857e3f9f654df457234173/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L646

But as part of adding (4b8c5fe#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R1180) and then reverting (3609f05#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R1271), this Socket-tracking code was (presumably accidentally) removed for .NET 5, with it now only checking for NetworkStream:
3609f05#diff-595f700a4c83b735d856fe14b5ebe8c7f7c18ae6e586adc8b398a997a0678274R82-R85
This means that whereas in .NET Core 3.1 we would only poll rather than read-ahead for https connections when periodically checking their status in the pool, now in .NET 5 we're doing read-aheads for https connections, and incurring the aforementioned problems that come with it (buffers pinned for long periods, extra exceptions when connections are torn down).

This was recently compounded for .NET 6 as part of #48446 (comment), but the core of the problem was introduced in .NET 5.

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

[stephentoub]

Reopening to track porting back to 5.0

[karelz]

Fixed in 6.0 by PR #49474 and in 5.0.6 by PR #50776.