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.

Sign up for GitHub

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

Jump to bottom

Avoid capturing the ExecutionContext for the whole HTTP connection lifetime #86213

Closed
wants to merge 1 commit into from
Closed

Avoid capturing the ExecutionContext for the whole HTTP connection lifetime #86213

wants to merge 1 commit into from

Conversation

MihaZupan
Copy link
Member

Fixes #80232

@MihaZupan MihaZupan added this to the 8.0.0 milestone May 13, 2023
@MihaZupan MihaZupan requested a review from a team May 13, 2023 22:08
@MihaZupan MihaZupan self-assigned this May 13, 2023
@ghost
Copy link

ghost commented May 13, 2023

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

Issue Details

Fixes #80232

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http
Milestone: 8.0.0

@davidfowl
Copy link
Member

This isn’t going to break any of the crazy code we gave to teams to associate connections with requests using async locals right?

@MihaZupan
Copy link
Member Author

If you mean hacks like #63159 (comment), those will still work as the connection itself is still used within the async scope of the request's SendAsync (write calls on HTTP/1 at least, which is what that hack relies on).

For things like the connection establishment itself, this PR doesn't change anything, and we have some tests that verify events like ConnectionEstablished happen within the request's scope.

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request May 14, 2023
Closed
@MihaZupan
Copy link
Member Author

The HTTP/3 test failure is related. Either a test bug or there's still some place where we capture the context there. Setting to draft for now before I get to investigate.

@MihaZupan MihaZupan marked this pull request as draft May 25, 2023 12:50
@ghost
Copy link

ghost commented Jun 24, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost closed this Jun 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@MihaZupan MihaZupan

Labels
area-System.Net.Http
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Http2Connection keeps the ExecutionContext of the first request alive
3 participants
@MihaZupan @davidfowl @stephentoub