Skip to content

Remove workaround in HeaderPropagationMiddleware#20533

Merged
halter73 merged 1 commit intomasterfrom
halter73/remove-header-propagation-workaround
Apr 9, 2020
Merged

Remove workaround in HeaderPropagationMiddleware#20533
halter73 merged 1 commit intomasterfrom
halter73/remove-header-propagation-workaround

Conversation

@halter73
Copy link
Member

@halter73 halter73 commented Apr 4, 2020

Fixed by #14146 which starts each request on a fresh ExecutionContext

Fixed by #14146 which starts each request on a fresh ExecutionContext
@ghost ghost added the area-middleware label Apr 4, 2020
@halter73 halter73 requested a review from davidfowl April 4, 2020 01:02
}

[Fact]
public async Task HeaderInRequest_WithBleedAsyncLocal_HasCorrectValue()
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed these tests because it's the responsibility of the server, not the middleware, to ensure it has a fresh ExecutionContext for each request. The HeaderPropagationMiddleware does not have any functional tests, and @benaadams already added functional tests to verify IAsyncLocals in general work as expected with Kestrel.

That said, I did manually verify that #14146 fixes things so that the HeaderPropagationMiddleware sees a fresh HeaderPropagationValues each request after removing async from Invoke. And verified this was not the case without the changes from #14146.

@halter73 halter73 merged commit 5607ea1 into master Apr 9, 2020
@halter73 halter73 deleted the halter73/remove-header-propagation-workaround branch April 9, 2020 23:43
@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares 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 middlewares

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants