Skip to content

Support LF request line terminator in HttpParser #43202

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

Merged
merged 15 commits into from
Aug 19, 2022

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Aug 10, 2022

Adding support for https://www.rfc-editor.org/rfc/rfc7230.html#section-3.5 which can be disabled via an app switch.

Although the line terminator for the start-line and header fields is
the sequence CRLF, a recipient MAY recognize a single LF as a line
terminator and ignore any preceding CR.

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 changing the HTTP parser to support it by default, and an optional switch to disable the support.

Regression?

No

Risk:

  • Some previously rejected requests might now (correctly) be accepted.
  • Might introduce bugs in the http parser (c.f. verification).
  • Performance regressions (c.f. verification).

Verification:

Automated:

  • Existing comprehensive unit tests.
  • Added new set of tests for LF terminators.
  • Ensured 100% code coverage for the parser.

Manual:

  • App Service has been testing this change successfully for months.
  • TE Benchmarks.

@sebastienros
Copy link
Member Author

Deprecates #41269

@adityamandaleeka adityamandaleeka self-assigned this Aug 11, 2022
@adityamandaleeka
Copy link
Member

I just pushed another commit that reworks the HttpParser changes, bringing back the perf benefits afforded by the IndexOfAny in the common CR/LF case.

Benchmarking with both a local microbenchmark and with the plaintext benchmark now show no perf impact with this change. Here's the plaintext delta before and after this PR:

| application           | base                 | pr                   |         |
| --------------------- | -------------------- | -------------------- | ------- |
| CPU Usage (%)         |                  100 |                   99 |  -1.00% |
| Cores usage (%)       |                2,786 |                2,779 |  -0.25% |
| Working Set (MB)      |                  193 |                  193 |   0.00% |
| Private Memory (MB)   |                1,165 |                1,165 |   0.00% |
| Build Time (ms)       |                2,161 |                1,641 | -24.06% |
| Start Time (ms)       |                  224 |                  217 |  -3.12% |
| Published Size (KB)   |               97,788 |               97,788 |   0.00% |
| .NET Core SDK Version | 7.0.100-rc.1.22411.3 | 7.0.100-rc.1.22411.3 |         |


| load                   | base        | pr          |         |
| ---------------------- | ----------- | ----------- | ------- |
| CPU Usage (%)          |          92 |          93 |  +1.09% |
| Cores usage (%)        |       2,565 |       2,596 |  +1.21% |
| Working Set (MB)       |          38 |          38 |   0.00% |
| Private Memory (MB)    |         370 |         370 |   0.00% |
| Start Time (ms)        |           0 |           0 |         |
| First Request (ms)     |          73 |          74 |  +1.37% |
| Requests/sec           |  10,757,739 |  10,795,674 |  +0.35% |
| Requests               | 162,412,672 | 163,000,536 |  +0.36% |
| Mean latency (ms)      |        1.39 |        1.29 |  -7.19% |
| Max latency (ms)       |       74.00 |       64.43 | -12.93% |
| Bad responses          |           0 |           0 |         |
| Socket errors          |           0 |           0 |         |
| Read throughput (MB/s) |    1,290.24 |    1,300.48 |  +0.79% |
| Latency 50th (ms)      |        0.77 |        0.77 |  -0.65% |
| Latency 75th (ms)      |        1.18 |        1.16 |  -1.69% |
| Latency 90th (ms)      |        2.01 |        1.86 |  -7.46% |
| Latency 99th (ms)      |       15.40 |       14.24 |  -7.53% |

As good as the latency deltas look, they are within the normal noise range for this test and can be ignored.

@adityamandaleeka adityamandaleeka removed their assignment Aug 12, 2022
@adityamandaleeka
Copy link
Member

@sebastienros I can't add you as a reviewer haha. I showed you most of this stuff on my screen but please take another look at my commit.

@sebastienros sebastienros enabled auto-merge (squash) August 12, 2022 19:57
@wtgodbe
Copy link
Member

wtgodbe commented Aug 12, 2022

@sebastienros can you open an issue with an API proposal & send it to the API reviewers email list so this can get approved?

@sebastienros sebastienros disabled auto-merge August 12, 2022 20:48
@sebastienros sebastienros self-assigned this Aug 12, 2022
@sebastienros
Copy link
Member Author

Renaming applied, and test code coverage is complete.

@sebastienros sebastienros changed the base branch from main to release/7.0-rc1 August 17, 2022 17:33
@sebastienros
Copy link
Member Author

Benchmarks with LF support enabled by default in 7.0 vs. without LF support is good:

| application           | main                 | pr                   |        |
| --------------------- | -------------------- | -------------------- | ------ |
| CPU Usage (%)         |                   99 |                   99 |  0.00% |
| Cores usage (%)       |                2,769 |                2,783 | +0.51% |
| Working Set (MB)      |                  191 |                  193 | +1.05% |
| Private Memory (MB)   |                1,165 |                1,168 | +0.26% |
| Build Time (ms)       |                1,542 |                1,535 | -0.45% |
| Start Time (ms)       |                  222 |                  205 | -7.66% |
| Published Size (KB)   |               98,034 |               98,034 |  0.00% |
| .NET Core SDK Version | 7.0.100-rc.2.22417.1 | 7.0.100-rc.2.22417.1 |        |


| load                   | main        | pr          |         |
| ---------------------- | ----------- | ----------- | ------- |
| CPU Usage (%)          |          91 |          92 |  +1.10% |
| Cores usage (%)        |       2,554 |       2,562 |  +0.31% |
| Working Set (MB)       |          38 |          38 |   0.00% |
| Private Memory (MB)    |         370 |         370 |   0.00% |
| Start Time (ms)        |           0 |           0 |         |
| First Request (ms)     |          72 |          77 |  +6.94% |
| Requests/sec           |  10,673,287 |  10,726,641 |  +0.50% |
| Requests               | 161,164,728 | 161,969,368 |  +0.50% |
| Mean latency (ms)      |        1.32 |        1.43 |  +8.33% |
| Max latency (ms)       |       61.53 |       86.32 | +40.29% |
| Bad responses          |           0 |           0 |         |
| Socket errors          |           0 |           0 |         |
| Read throughput (MB/s) |    1,280.00 |    1,290.24 |  +0.80% |
| Latency 50th (ms)      |        0.78 |        0.78 |  -0.51% |
| Latency 75th (ms)      |        1.18 |        1.19 |  +0.85% |
| Latency 90th (ms)      |        1.90 |        2.04 |  +7.37% |
| Latency 99th (ms)      |       14.50 |       15.82 |  +9.10% |

@sebastienros sebastienros changed the title Add LF request line terminator option Support LF request line terminator in HttpParser Aug 17, 2022
@sebastienros
Copy link
Member Author

Fixes #43252

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Looks great!

@adityamandaleeka
Copy link
Member

This is approved for RC1. 🎉

@sebastienros
Copy link
Member Author

@adityamandaleeka you only can merge, thanks

@adityamandaleeka adityamandaleeka merged commit a81d189 into release/7.0-rc1 Aug 19, 2022
@adityamandaleeka adityamandaleeka deleted the sebros/lf branch August 19, 2022 23:50
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants