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

[release/6.0] Allow retaining the original casing for known header values #80754

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jan 17, 2023

Backport of #64702 to release/6.0, gated behind an AppContext switch.

Customer Impact

Starting with .NET Core 2.1 and later expanded in .NET 5.0, we introduced a performance optimization where we avoid allocating strings for common well-known header values.
We did this in a case-insensitive manner, so if the server sent us Connection: close or Connection: CLOSE, they would both be exposed to the user as close.

As services are migrating from Framework to Core, this represents a change in behavior that can't be avoided as the optimization is taking place lower in the stack before the information is exposed to the user.
Clients running different versions of .NET may therefore not agree on the exact response provided by the server, which can break scenarios such as signature validation.

We stopped doing case-insensitive matching in .NET 7.0, and this PR brings the option to do the same on the 6.0 LTS release to unblock migration.

Testing

I added a targeted test to verify the expected behavior is observed if the AppContext switch is set.
The customer validated the new behavior using a private build of System.Net.Http.

Risk

Practically zero. The change in behavior is hidden behind an AppContext switch.

@MihaZupan MihaZupan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Http labels Jan 17, 2023
@MihaZupan MihaZupan added this to the 6.0.x milestone Jan 17, 2023
@MihaZupan MihaZupan self-assigned this Jan 17, 2023
@ghost
Copy link

ghost commented Jan 17, 2023

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

Issue Details

Backport of #64702 to release/6.0, gated behind an AppContext switch.

Set to draft pending customer validation.

Customer Impact

TODO

Testing

TODO

Risk

TODO

Author: MihaZupan
Assignees: MihaZupan
Labels:

NO-MERGE, area-System.Net.Http

Milestone: 6.0.x

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan MihaZupan marked this pull request as ready for review January 20, 2023 14:33
@MihaZupan MihaZupan added Servicing-consider Issue for next servicing release review Servicing-approved Approved for servicing release and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) Servicing-consider Issue for next servicing release review labels Jan 20, 2023
@MihaZupan
Copy link
Member Author

MihaZupan commented Jan 23, 2023

Approved by @SteveMCarroll on 1/23 via email - "[Servicing-Consider] [6.0.X] Allow retaining the original casing for known header values".

@MihaZupan
Copy link
Member Author

MihaZupan commented Jan 31, 2023

Failures are #80628, #81391, #81544

@carlossanlop
Copy link
Member

Approved by Tactics for 6.0.15.
Signed-off by area owner.
No OOB changes needed for System.Net.Http.
CI failures are unrelated: #81544 and pre-existing tvos simulator failures.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 955f05e into dotnet:release/6.0 Feb 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants