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

HeaderPropagation: reset AsyncLocal per request #15435

Closed
wants to merge 1 commit into from

Conversation

alefranz
Copy link
Contributor

@alefranz alefranz commented Oct 26, 2019

As Kestrel can bleed the AsyncLocal across requests,
see #13991.

Addresses #15384

/cc @benaadams @davidfowl

@benaadams
Copy link
Member

Trouble with this is if there is an httprequest that happens with lag on a previous request due to fire and forgot (which would be likely problematic anyway) it will pick up the changed dictionary from the async local.

Either would need to make the Invoke method async which would detach the asynclocal; or make the base request method async to detach all asynclocals #14146

@alefranz
Copy link
Contributor Author

Thanks Ben for the quick feedback.
I'll wait for the outcome of #14146.

@@ -36,7 +36,8 @@ public HeaderPropagationMiddleware(RequestDelegate next, IOptions<HeaderPropagat
public Task Invoke(HttpContext context)
Copy link
Member

Choose a reason for hiding this comment

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

I would also make this method async

As Kestrel can bleed the AsyncLocal across requests,
see dotnet#13991.
@alefranz
Copy link
Contributor Author

Changed approach and made the middleware async as suggested.
Not sure if it makes sense to go ahead with this change or wait for #14146 ?

@jkotalik
Copy link
Contributor

@davidfowl didn't you fix this in 5.0 in Kestrel?

@jkotalik jkotalik added this to the 5.0.0-preview1 milestone Oct 31, 2019
@halter73
Copy link
Member

Why not just take #14027 instead?

@benaadams
Copy link
Member

benaadams commented Oct 31, 2019

Why not just take #14027 instead?

Or #14146 which just makes ProcessRequest async so the changes are reset on each request

@alefranz
Copy link
Contributor Author

would it be possible to include this in 3.1 given it is a bugfix, and eventually remove it for 5.0 once one of the global fixes lands?

@analogrelay
Copy link
Contributor

We need to land on which of these we take, this PR or #14146 . Consensus in triage seems to lean towards #14146 .

As to backporting to 3.1, that will be open as an option once we merge to master. It's too late for 3.1.0 and this may be risky for a patch release, so we'll need to assess that separately.

@analogrelay
Copy link
Contributor

We're going to close this in lieu of #14146 . If that doesn't land, we can revisit.

@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants