Add custom request header decoder API to Kestrel #23233

halter73 · 2020-06-22T20:01:35Z

TODONE for this PR:
- Benchmarks. I want to make sure we agree on this design before benchmarking. In the fast path, it only adds a couple comparisons, so I think we should be good.

TODO for future PRs:
- URL decoding
- ANCM in-proc support

Partially Addresses #17400

src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

src/Shared/ServerInfrastructure/StringUtilities.cs

src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp.cs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

halter73 · 2020-06-26T20:59:26Z

Http1ConnectionBenchmark Microbenchmark Results

Before (release/5.0-preview7)

default (UTF-8) encoding

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.119 us	0.0222 us	0.0458 us	894,046.9	0.0210	-	-	880 B

latin1 (ISO-8859-1) encoding

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.171 us	0.0231 us	0.0549 us	854,282.0	0.0210	-	-	880 B

After (halter73/17400)

default (UTF-8) encoding

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.135 us	0.0180 us	0.0160 us	881,286.2	0.0229	-	-	880 B

latin1 (ISO-8859-1) encoding (`RequestHeaderEncodingSelector = _ => Encoding.Latin1`)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.172 us	0.0217 us	0.0203 us	853,229.7	0.0210	-	-	880 B

Alternative default (UTF-8) encoding (`RequestHeaderEncodingSelector = _ => null`)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.146 us	0.0166 us	0.0155 us	872,465.7	0.0229	-	-	880 B

Alternative defaultish (UTF-8) encoding (`RequestHeaderEncodingSelector = _ => Encoding.UTF8`)

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
LiveAspNet	1.310 us	0.0173 us	0.0153 us	763,159.7	0.0229	-	-	880 B

Given that we're trying to not regress perf rather than improve it with this PR, this all looks reasonable to me.

I made sure to set DisableStringReuse = true and hard-coded the "runtimeFrameworkVersion" to 5.0.0-preview.8.20323.9 for all benchmark runs. See #23000 (comment)

halter73 · 2020-06-26T20:59:38Z

@Pilchie Can you merge this?

davidfowl · 2020-06-26T21:07:55Z

Can you run a CPU profile to see if there's anything that stands out?

davidfowl · 2020-06-26T21:09:03Z

Also can you run the PR benchmark? This is basically in the hot path.

halter73 · 2020-06-26T22:01:26Z

Can you run a CPU profile to see if there's anything that stands out?

I don't have time for this today, and I still want this in preview7. It wouldn't be that useful yet anyway, since in the default case, we do 1 extra ReferenceEquals() comparison per request header and follow the old request decoding path. I don't need a CPU profile to tell me that.

halter73 · 2020-06-26T22:08:54Z

@aspnet-hello benchmark

pr-benchmarks · 2020-06-27T00:14:02Z

Starting 'Default' pipelined plaintext benchmark with session ID 'cf5b7aeeed484d849f9228c166698b36'. This could take up to 30 minutes...

pr-benchmarks · 2020-06-27T00:15:45Z

Baseline

stdout: Starting baseline run on '4465a8efc853ffb0062e316032e5e907e04736a6'...
The specified server url 'http://10.0.0.9:5001' is invalid or not responsive.
[12:15:44.889] Interrupting due to an unexpected exception
[12:15:44.911] System.Net.Http.HttpRequestException: Connection refused
 ---> System.Net.Sockets.SocketException (111): Connection refused
   at System.Net.Http.ConnectHelper.ConnectAsync(String host, Int32 port, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.ConnectHelper.ConnectAsync(String host, Int32 port, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.ConnectAsync(HttpRequestMessage request, Boolean allowHttp2, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.CreateHttp11ConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.GetHttpConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.DecompressionHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts)
   at BenchmarksDriver.Program.Run(Uri serverUri, Uri[] clientUris, String sqlConnectionString, ServerJob serverJob, String session, String description, Int32 iterations, Int32 exclude, String shutdownEndpoint, TimeSpan span, List`1 downloadFiles, Boolean fetch, String fetchDestination, Boolean collectR2RLog, String traceDestination, CommandOption outputFileOption, CommandOption sourceOption, CommandOption scriptFileOption, CommandOption markdownOption, CommandOption writeToFileOption, Nullable`1 requiredOperatingSystem, CommandOption archOption, CommandOption saveOption, CommandOption diffOption) in /app/src/BenchmarksDriver/Program.cs:line 1445


stderr: Baseline benchmark run on '4465a8efc853ffb0062e316032e5e907e04736a6' failed.

PR

davidfowl

Watch the benchmarks and lets make sure we profile this

halter73 · 2020-06-27T00:24:53Z

I'm also talking to @sebastienros about the benchmark server issue. I agree with him that we should move over to the new driver so we can use the same servers as everything else. We just need to budget time to make the changes.

sebastienros · 2020-06-27T00:43:07Z

@aspnet-hello benchmark

pr-benchmarks · 2020-06-27T00:50:27Z

Starting 'Default' pipelined plaintext benchmark with session ID '1357ac5cf4f14d2c80c16f4120a48094'. This could take up to 30 minutes...

pr-benchmarks · 2020-06-27T00:53:52Z

Baseline

Starting baseline run on '4465a8efc853ffb0062e316032e5e907e04736a6'...
RequestsPerSecond:           791,229
Max CPU (%):                 99
WorkingSet (MB):             88
Avg. Latency (ms):           3.11
Startup (ms):                534
First Request (ms):          134.75
Latency (ms):                0.4
Total Requests:              11,889,004
Duration: (ms)               15,030
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             22,045
Published Size (KB):         120,307
SDK:                         5.0.100-preview.6.20310.4
Runtime:                     5.0.0-preview.8.20326.2
ASP.NET Core:                5.0.0-preview.8.20326.8

PR

Starting PR run on 'b9404b03ad150bbeb3c99b19aabf716969c5bbb5'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 791,229 |      99 |          88 |              3.11 |          534 |           22045 |              120307 |             134.75 |          0.4 |      0 |  1.00 |
|       After | 798,803 |      99 |          87 |              3.24 |          503 |            6002 |              120307 |             127.88 |         0.34 |      7 |  1.01 |

halter73 added area-servers feature-kestrel api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 22, 2020

halter73 added this to the 5.0.0-preview7 milestone Jun 22, 2020

halter73 requested review from Tratcher and jkotalik June 22, 2020 20:01

Tratcher reviewed Jun 22, 2020

View reviewed changes

halter73 force-pushed the halter73/17400 branch from e076ec2 to 8bbb7f2 Compare June 22, 2020 21:31

dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Jun 23, 2020

halter73 force-pushed the halter73/17400 branch from eec64d3 to 57c26b2 Compare June 24, 2020 01:06

Tratcher approved these changes Jun 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs Show resolved Hide resolved

davidfowl reviewed Jun 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Show resolved Hide resolved

davidfowl reviewed Jun 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Show resolved Hide resolved

davidfowl reviewed Jun 24, 2020

View reviewed changes

src/Shared/ServerInfrastructure/StringUtilities.cs Show resolved Hide resolved

davidfowl reviewed Jun 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp.cs Show resolved Hide resolved

halter73 added 7 commits June 24, 2020 12:29

Add custom request header decoder API to Kestrel

27f60a4

Add tests

602353b

Improve doc comment

7c34ed1

Update ref assembly

032d02e

Address PR feedback

d009eb1

Remove nullable annotations for now

4cf663a

Cleanup whitespace and pass inner exception

39f8ada

halter73 force-pushed the halter73/17400 branch from 57c26b2 to b07ac51 Compare June 24, 2020 19:31

Remove "Latin1RequestHeaders" config

b9404b0

halter73 force-pushed the halter73/17400 branch from b07ac51 to b9404b0 Compare June 25, 2020 00:45

pranavkm reviewed Jun 25, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Show resolved Hide resolved

halter73 added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 26, 2020

dotnet deleted a comment from pr-benchmarks bot Jun 26, 2020

dotnet deleted a comment from pr-benchmarks bot Jun 27, 2020

davidfowl approved these changes Jun 27, 2020

View reviewed changes

Pilchie merged commit b446ab7 into release/5.0-preview7 Jun 27, 2020

Pilchie deleted the halter73/17400 branch June 27, 2020 02:49

halter73 mentioned this pull request Sep 21, 2020

Document Kestrel breaking changes, new API and new config in 5.0 #26141

Closed

Tratcher mentioned this pull request Sep 14, 2021

Kestrel rejects requests with non standard ASCII characters #36509

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom request header decoder API to Kestrel #23233

Add custom request header decoder API to Kestrel #23233

halter73 commented Jun 22, 2020 •

edited

Loading

halter73 commented Jun 26, 2020

halter73 commented Jun 26, 2020

davidfowl commented Jun 26, 2020

davidfowl commented Jun 26, 2020

halter73 commented Jun 26, 2020 •

edited

Loading

halter73 commented Jun 26, 2020

pr-benchmarks bot commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

davidfowl left a comment

halter73 commented Jun 27, 2020

sebastienros commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

Add custom request header decoder API to Kestrel #23233

Add custom request header decoder API to Kestrel #23233

Conversation

halter73 commented Jun 22, 2020 • edited Loading

halter73 commented Jun 26, 2020

Http1ConnectionBenchmark Microbenchmark Results

Before (release/5.0-preview7)

default (UTF-8) encoding

latin1 (ISO-8859-1) encoding

After (halter73/17400)

default (UTF-8) encoding

latin1 (ISO-8859-1) encoding (RequestHeaderEncodingSelector = _ => Encoding.Latin1)

Alternative default (UTF-8) encoding (RequestHeaderEncodingSelector = _ => null)

Alternative defaultish (UTF-8) encoding (RequestHeaderEncodingSelector = _ => Encoding.UTF8)

halter73 commented Jun 26, 2020

davidfowl commented Jun 26, 2020

davidfowl commented Jun 26, 2020

halter73 commented Jun 26, 2020 • edited Loading

halter73 commented Jun 26, 2020

pr-benchmarks bot commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

Baseline

PR

davidfowl left a comment

Choose a reason for hiding this comment

halter73 commented Jun 27, 2020

sebastienros commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

pr-benchmarks bot commented Jun 27, 2020

Baseline

PR

halter73 commented Jun 22, 2020 •

edited

Loading

latin1 (ISO-8859-1) encoding (`RequestHeaderEncodingSelector = _ => Encoding.Latin1`)

Alternative default (UTF-8) encoding (`RequestHeaderEncodingSelector = _ => null`)

Alternative defaultish (UTF-8) encoding (`RequestHeaderEncodingSelector = _ => Encoding.UTF8`)

halter73 commented Jun 26, 2020 •

edited

Loading