Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix polling on https connections in HttpConnectionPool #49474

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

stephentoub
Copy link
Member

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
Copy link

ghost commented Mar 11, 2021

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: -

@stephentoub stephentoub force-pushed the fixhttppinning branch 2 times, most recently from 48c65ec to e345db3 Compare March 11, 2021 11:29
Avoid performing read-aheads on such connections, which can result in long-pinned buffers and unnecessary faulted tasks when the connections are torn down.
@scalablecory
Copy link
Contributor

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.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

wfurt commented Mar 11, 2021

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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?

…andler/HttpConnectionPool.cs

Co-authored-by: Cory Nelson <phrosty@gmail.com>
@wfurt
Copy link
Member

wfurt commented Mar 11, 2021

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 stephentoub merged commit d1da078 into dotnet:main Mar 12, 2021
@stephentoub stephentoub deleted the fixhttppinning branch March 12, 2021 01:41
@stephentoub
Copy link
Member Author

/backport to release/5.0

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

@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!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 11, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpConnection regression around read-aheads on pooled https connections
5 participants