Skip to content

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
Contributor

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.

8 participants