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

Simplify Http2Connection shutdown/dispose logic #90094

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

MihaZupan
Copy link
Member

Closes #84767

Context: #84767 (comment)

There's a race here where we'll signal the shutdown waiter and set the dispose flag at the same time, but since SignalShutdownWaiter means "push a work item to the thread pool", the connection may not be removed from the pool immediately, so things can still see the connection on the pool in a disposed state.

I see at least two ways we could avoid this:

  • Instead of all the "ShutdownWaiter" logic, why aren't we just calling pool.InvalidateHttp2Connection when disposing/shutting down the Http2Connection? Feels like a lot of extra code for an indirection?
  • Why do we differentiate between _shutdown and _disposed in the Http2Connection? I may be missing some detail, but it looks like Dispose could just call Shutdown?

This PR merges together the Dispose & Shutdown logic (dispose calls shutdown) since as far as I can see, they were doing the same thing.

I also removed the ShutdownWaiter logic coordinating between the pool and connection, calling InvalidateHttp2Connection directly instead.

@MihaZupan MihaZupan added this to the 8.0.0 milestone Aug 7, 2023
@MihaZupan MihaZupan requested a review from a team August 7, 2023 12:47
@MihaZupan MihaZupan self-assigned this Aug 7, 2023
@ghost
Copy link

ghost commented Aug 7, 2023

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

Issue Details

Closes #84767

Context: #84767 (comment)

There's a race here where we'll signal the shutdown waiter and set the dispose flag at the same time, but since SignalShutdownWaiter means "push a work item to the thread pool", the connection may not be removed from the pool immediately, so things can still see the connection on the pool in a disposed state.

I see at least two ways we could avoid this:

  • Instead of all the "ShutdownWaiter" logic, why aren't we just calling pool.InvalidateHttp2Connection when disposing/shutting down the Http2Connection? Feels like a lot of extra code for an indirection?
  • Why do we differentiate between _shutdown and _disposed in the Http2Connection? I may be missing some detail, but it looks like Dispose could just call Shutdown?

This PR merges together the Dispose & Shutdown logic (dispose calls shutdown) since as far as I can see, they were doing the same thing.

I also removed the ShutdownWaiter logic coordinating between the pool and connection, calling InvalidateHttp2Connection directly instead.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 8.0.0

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Test failures are #89173, #90121, #90125

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. But I'm not too familiar with this logic.
Do you know if we have tests to cover the cases when request/response should working after Dispose?

@MihaZupan
Copy link
Member Author

I really thought we already had such a test, but I don't see it now.

We have a different one where we dispose the HttpClient and test that the request will be torn down.

I added a new test now that verifies that disposing the handler during a request is fine.

@MihaZupan MihaZupan merged commit 910782e into dotnet:main Aug 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
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.

System.Net.Http.Functional.Tests failure
2 participants