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

Tear down pending HTTP connection when the originating request completes #71785

Merged
merged 35 commits into from
Jul 26, 2022

Conversation

antonfirsov
Copy link
Member

Resolves #66297

/cc @stephentoub

@antonfirsov antonfirsov added this to the 7.0.0 milestone Jul 7, 2022
@ghost ghost assigned antonfirsov Jul 7, 2022
@ghost
Copy link

ghost commented Jul 7, 2022

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

Issue Details

Resolves #66297

/cc @stephentoub

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http

Milestone: 7.0.0

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

@stephentoub @dotnet/ncl this is ready for another round of reviews.

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.

Thanks. It'd be good to do some stress testing on this.

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 21, 2022
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.

Assuming CI and http stress look good, LGTM.

# Conflicts:
#	src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
@antonfirsov
Copy link
Member Author

Assuming CI and http stress look good

finally
{
// We never cancel both attempts at the same time. When downgrade happens, it's possible that both waiters are non-null,
// but in that case http2ConnectionWaiter.ConnectionCancellationTokenSource shall be null.
Debug.Assert(http11ConnectionWaiter is null || http2ConnectionWaiter?.ConnectionCancellationTokenSource is null);

^ this assertion failed in one of the runs, which I addressed in dbdbbc3. If there is no pushback against that change and the next CI run is OK, I will merge.

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 25, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 25, 2022
@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov antonfirsov reopened this Jul 26, 2022
@antonfirsov
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

Test failures are unrelated, System.Net.Http failure is #72818 / #72586.

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.

Change SocketsHttpHandler's default ConnectTimeout to a reasonable finite value
4 participants