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

HTTP3: Dispose HTTP3 connection when HttpClientHandler is disposed #56943

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

geoffkizer
Copy link
Contributor

Fixes #56785

@geoffkizer geoffkizer added this to the 6.0.0 milestone Aug 5, 2021
@geoffkizer geoffkizer requested review from ManickaP and wfurt August 5, 2021 23:55
@ghost
Copy link

ghost commented Aug 5, 2021

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

Issue Details

Fixes #56785

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: 6.0.0

@wfurt
Copy link
Member

wfurt commented Aug 6, 2021

Would this be only when there are no pending requests/responses or always?
I though we preserve the HttpResponseStreams but maybe I'm mistaken with something else.

@geoffkizer
Copy link
Contributor Author

We do preserve HttpResponseStreams, but that's the responsibility of the Http3Connection to do it -- in other words, when you call Dispose on it, it will initiate client-based shutdown and close the connection when the outstanding requests are completed.

Note HTTP2 works the same way -- we dispose all the HTTP2 connections, even though some may have in flight requests.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

src update LGTM

@wfurt
Copy link
Member

wfurt commented Aug 6, 2021

Ok. So this will not automatically call QuicConnection.Dispose, right? (that would rest all the streams)

@geoffkizer
Copy link
Contributor Author

So this will not automatically call QuicConnection.Dispose, right?

Right. Http3Connection.Dispose marks the connection as shutting down, then calls CheckForShutdown, which does the drain logic I described above.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

@geoffkizer geoffkizer merged commit 6ac0370 into dotnet:main Aug 6, 2021
@ManickaP
Copy link
Member

ManickaP commented Aug 6, 2021

@geoffkizer Is this fixing #56766 ? Because the link description points to a PR not an issue and this looks like work on #56766. If so we should close that issue.

@geoffkizer
Copy link
Contributor Author

@geoffkizer Is this fixing #56766 ?

Yes. Sorry, don't know how I ended up linking the wrong one.

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.

4 participants