-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add linefeed line terminator switch #41101
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
Conversation
Hi @sebastienros. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document. |
Really should start PRs on main since it's easier to remember to backport than it is to remember to forward port. |
I agree but it's more a question of deadline here. I needed to get a local build ready for testing before it is even merged. |
src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
ce84e1a
to
fad1f59
Compare
/backport to main |
Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/2192179026 |
This comment was marked as outdated.
This comment was marked as outdated.
Do we have any benchmarks with a bunch of request headers before and after? |
@sebastienros what is the status of this PR❔ I see neither a |
Plaintext platform with full pgo, default settings.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plaintext platform with full pgo, default settings.
For these kinds of changes, we should also be running the microbenchmarks in HttpParserBenchmark.cs.
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Outdated
Show resolved
Hide resolved
I'm HUGE fan of the updated code! It's a lot cleaner than it was before while also adding the new switch. Do we have any perf numbers for the latest changes? I suspect it's as fast or faster than before, but I want to verify. @dougbu I just discussed this with @sebastienros. We think the internal customer who wants this switch can test this out from an internal feed first. That would increase our confidence in these changes. As of right now, we've only been able to test this by manually crafting invalid requests that we think look like requests a real client might send. We haven't been able to test this with a request from a real client or even a replay of a request from a real client. |
Reminder that code complete is today. |
Removing |
Please let me know if and when this gets |
@dougbu Will submit a new request for next service release, and I also need to update it. It's currently being tested internally. |
At this point you also have conflicts. That'll help me remember not to merge until all t's are dotted and i's crossed 😃 |
We have a slight hold up in opening the release/6.0 branch but I'm wondering if this PR will be ready as soon as that happens. Currently hitting merge conflicts and I don't see a |
@sebastienros @adityamandaleeka are we trying to get this into 6.0.9❔ Need to resolve conflicts if so |
{ | ||
if (!_enableLineFeedTerminator.HasValue) | ||
{ | ||
_enableLineFeedTerminator = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableLineFeedTerminator", out var enabled) && enabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_enableLineFeedTerminator = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableLineFeedTerminator", out var enabled) && enabled; | |
_allowHttp1LineFeedTerminators = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.AllowHttp1LineFeedTerminators", out var enabled) && enabled; |
Please resolve the conflicts if this is going into |
Adding support for https://www.rfc-editor.org/rfc/rfc7230.html#section-3.5 via an app switch.
Description:
The HTTP specification allows servers to support a single LF as line terminator (on top of CR/LF). Azure App Service has been deploying Yarp as their front-end and faces clients trying to use this capability.
The PR is adding an optional switch to enable the feature.
Regression?
No
Risk:
The feature is enabled by a switch, existing applications are not impacted.
Verification:
Updated unit tests and verified by App Service.