Fix polling on https connections in HttpConnectionPool

[stephentoub]

Fixes #49431

Avoid performing read-aheads on such connections, which can result in long-pinned buffers and unnecessary exceptions when the connections are torn down.

cc: @geoffkizer, @scalablecory

[ghost]

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

Issue Details

---

Fixes #49431

Avoid performing read-aheads on such connections, which can result in long-pinned buffers and unnecessary exceptions when the connections are torn down.

cc: @geoffkizer, @scalablecory

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

[scalablecory]

Actually, before you merge this:

TLS can send alerts out of band from stream data right @wfurt? Presumably this "check that we haven't received anything" would fail with this change.

[wfurt]

LGTM. I'm wondering if long-term we can use the 0-read @geoffkizer recently fixed. It is somewhat sad that using Stream has negative impact.

[wfurt]

TLS can send alerts out of band from stream data right @wfurt? Presumably this "check that we haven't received anything" would fail with this change.

If the alert is coming from remote side, then yes. It can come at any time. That can be close notification or the peer did not like something we sent. Typically that also terminates the connection shortly after. We can also generate alert on our side as result of error condition.

My assumption this generally moves back to state we already had before NetworkStream.Socket was available, right @stephentoub ?

[stephentoub]

My assumption this generally moves back to state we already had before NetworkStream.Socket was available

This moves us back to the state we were in for .NET 3.1: all http and https connections, except for those via proxy tunnels, poll the underlying socket for connections in the pool. With this change, we'll now also do it if a NetworkStream is returned from ConnectCallback (which obviously wasn't available in 3.1).

[stephentoub]

Even if such an alert happens, it seems like the worst case here is a race condition where we try to use a connection that's about to be closed and then we end up needing to retry... but such a race condition already exists, which is why we already have said retry code. Yes?

[scalablecory]

If the only thing an alert can be used for is an error condition then lets merge away 👍. Do we know that TLS 1.3 is the same @wfurt?

[wfurt]

If the only thing an alert can be used for is an error condition then lets merge away 👍. Do we know that TLS 1.3 is the same @wfurt?

yes in regard to alerts. The biggest changes were in TLS handshake.

[stephentoub]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/644747685

[github-actions]

@stephentoub backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix polling on https connections in HttpConnectionPool
error: sha1 information is lacking or useless (src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix polling on https connections in HttpConnectionPool
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!