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

Use object indirection in HttpContextAccessor (#1066) #6036

Merged
merged 1 commit into from
Jan 15, 2019

Conversation

JunTaoLuo
Copy link
Contributor

Addresses #5144. This is backporting 49d785c to 2.2

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Note the branches aren't opening for a few weeks.

@Tratcher Tratcher requested a review from muratg December 19, 2018 22:07
@JunTaoLuo JunTaoLuo self-assigned this Dec 19, 2018
@JunTaoLuo
Copy link
Contributor Author

Description:

In 2.2 we made a change to require request's TraceIdentifier to match when retrieving the HttpContext from the HttpContextAccessor. The goal of the change was to reduce the chance of an invalid HttpContext from being retrieved once the request has ended. However, this caused a regression since if any middleware modifies the TraceIdentifier on the request, the HttpContextAccessor will return a null value for HttpContext. In 3.0 a different approach is used to mitigate the retrieval of invalid HttpContext without relying on the TraceIdentifier. As a side-effect, the regression in behaviour was also removed.

Customer Impact

As mentioned in #5144, any customers on 2.2 will not be able to retrieve HttpContext from HttpContextAccessor if they modify the TraceIdentifier of the request.

Regression?

Regression from 2.1.

Risk:

Low. The fix has already been made to 3.0, we are just backporting it to 2.2

@muratg muratg added this to the 2.2.2 milestone Dec 20, 2018
@muratg muratg added the Servicing-consider Shiproom approval is required for the issue label Dec 20, 2018
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

We should add a regression test for that case we missed.

@vivmishra
Copy link

vivmishra commented Dec 20, 2018

Approved for 2.2.2.
Wait for branch to open.

@JunTaoLuo JunTaoLuo added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Dec 20, 2018
@dougbu
Copy link
Member

dougbu commented Jan 14, 2019

Before merging, please update the 2.2.2 section of eng/PatchConfig.props: https://github.com/aspnet/Extensions/blob/d8407116d183debaadb801c0cbc41d65927999d6/eng/PatchConfig.props#L7-L10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants