Kestrel response header encoding #33776

Tratcher · 2021-06-22T23:41:55Z

Fixes #4727

I have all of the plumbing in place with tests written and passing, but there's plenty of room to clean it up.

Added Http3HeadersEncoder for consistency with HTTP/2.
Response.Headers validation has been relaxed if you're using a non-default encoder. Control characters are still blocked, but non-ascii characters will be allowed.
- If the custom encoder decides to throw an exception when serializing the response rather than using a default replacement character, that will cause the connection to be aborted (HTTP/1 & 2, not sure about 3) to avoid inconsistent states like out of sync dynamic encoding. Applications can filter response headers in middleware/OnResponseStarting to avoid this.
Fixed a few spots where the request header encoding wasn't null checked.

TODO:

Sync HPack changes back to runtime

cc: @benaadams

ResponseHeadersWritingBenchmark (HTTP/1)

Before:

Method	Type	Mean	Error	StdDev	Median	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Output	TechEmpowerPlaintext	1.403 us	0.0321 us	0.0874 us	1.400 us	712,510.4	-	-	-	144 B
Output	PlaintextChunked	1.241 us	0.0287 us	0.0786 us	1.200 us	805,555.6	-	-	-	144 B
Output	PlaintextWithCookie	1.617 us	0.0609 us	0.1748 us	1.600 us	618,489.6	-	-	-	144 B
Output	Plain(...)ookie [26]	1.635 us	0.0367 us	0.0906 us	1.600 us	611,724.7	-	-	-	144 B
Output	LiveAspNet	3.493 us	0.0687 us	0.1257 us	3.500 us	286,298.6	-	-	-	144 B

After, default encoder:

Method	Type	Mean	Error	StdDev	Median	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Output	TechEmpowerPlaintext	1.563 us	0.0352 us	0.0720 us	1.600 us	639,899.6	-	-	-	144 B
Output	PlaintextChunked	1.468 us	0.0332 us	0.0476 us	1.500 us	681,265.2	-	-	-	144 B
Output	PlaintextWithCookie	1.934 us	0.0467 us	0.1332 us	1.900 us	517,051.7	-	-	-	144 B
Output	Plain(...)ookie [26]	2.051 us	0.0397 us	0.0736 us	2.100 us	487,528.3	-	-	-	144 B
Output	LiveAspNet	3.661 us	0.0780 us	0.2162 us	3.700 us	273,173.7	-	-	-	144 B

After, utf8 encoder:

Method	Type	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Output	TechEmpowerPlaintext	1.806 us	0.0482 us	0.1344 us	553,846.2	-	-	-	144 B
Output	PlaintextChunked	1.810 us	0.0545 us	0.1573 us	552,359.0	-	-	-	144 B
Output	PlaintextWithCookie	2.118 us	0.0493 us	0.1390 us	472,036.9	-	-	-	144 B
Output	Plain(...)ookie [26]	2.103 us	0.0578 us	0.1660 us	475,475.5	-	-	-	144 B
Output	LiveAspNet	3.877 us	0.0992 us	0.2781 us	257,936.5	-	-	-	144 B

HPackHeaderWriterBenchmark (Http/2)

Before:

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
BeginEncodeHeaders_KnownHeaders	636.2 ns	1.89 ns	1.68 ns	1,571,833.8	-	-	-	-
BeginEncodeHeaders_UnknownHeaders	724.9 ns	4.26 ns	3.78 ns	1,379,445.1	-	-	-	-

After:

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
BeginEncodeHeaders_KnownHeaders	682.1 ns	2.02 ns	1.89 ns	1,465,996.9	-	-	-	-
BeginEncodeHeaders_KnownHeaders_CustomEncoding	816.1 ns	2.87 ns	2.69 ns	1,225,306.0	-	-	-	-
BeginEncodeHeaders_UnknownHeaders	773.0 ns	2.75 ns	2.29 ns	1,293,619.5	-	-	-	-
BeginEncodeHeaders_UnknownHeaders_CustomEncoding	750.2 ns	3.03 ns	2.83 ns	1,333,002.7	-	-	-	-

Crank Plaintext default encoding:

application	old	new
CPU Usage (%)	100	99	-1.00%
Cores usage (%)	1,194	1,193	-0.08%
Working Set (MB)	72	71	-1.39%
Private Memory (MB)	395	370	-6.33%
Build Time (ms)	2,951	2,993	+1.42%
Start Time (ms)	162	202	+24.69%
Published Size (KB)	106,170	106,170	0.00%
.NET Core SDK Version	6.0.100-preview.6.21276.12	6.0.100-preview.6.21276.12

load	old	new
CPU Usage (%)	42	43	+2.38%
Cores usage (%)	503	513	+1.99%
Working Set (MB)	38	38	0.00%
Private Memory (MB)	358	358	0.00%
Start Time (ms)	0	0
First Request (ms)	97	149	+53.61%
Requests/sec	2,410,464	2,342,838	-2.81%
Requests	36,397,228	35,375,101	-2.81%
Mean latency (ms)	1.17	1.14	-2.56%
Max latency (ms)	49.73	41.52	-16.51%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	303.44	294.93	-2.80%
Latency 50th (ms)	0.95	0.97	+2.11%
Latency 75th (ms)	1.39	1.41	+1.44%
Latency 90th (ms)	1.79	1.80	+0.56%
Latency 99th (ms)	0.00	0.00

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

src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.cs

Tratcher · 2021-06-25T22:34:07Z

/AzurePipelines run AspNetCore-benchmarking-pr

azure-pipelines · 2021-06-25T22:34:13Z

No pipelines are associated with this pull request.

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

src/Servers/Kestrel/Core/src/Internal/Http/HttpResponseHeaders.cs

src/Servers/Kestrel/Core/src/Internal/Http3/Http3HeadersEnumerator.cs

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

JunTaoLuo

First batch

src/Servers/Kestrel/shared/test/StreamBackedTestConnection.cs

src/Shared/runtime/Http2/Hpack/HeaderField.cs

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

src/Servers/Kestrel/Core/src/Internal/Http/HttpResponseHeaders.cs

JunTaoLuo · 2021-06-30T17:34:24Z

src/Servers/Kestrel/Core/src/Internal/Http3/Http3HeadersEnumerator.cs

+
+        internal static int GetResponseHeaderStaticTableId(KnownHeaderType responseHeaderType)
+        {
+            // Not Implemented


When will we need to do this? Do we need to file a followup issue?

JunTaoLuo

Will circle back on reviewing tests. But src changes LGTM

src/Shared/ServerInfrastructure/BufferExtensions.cs

src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs

src/Servers/Kestrel/Core/src/Internal/Http3/Http3FrameWriter.cs

halter73 · 2021-06-30T19:13:59Z

src/Servers/Kestrel/Core/src/Internal/Http3/Http3HeadersEnumerator.cs

+
+        public Func<string, Encoding?> EncodingSelector { get; set; } = KestrelServerOptions.DefaultHeaderEncodingSelector;
+
+        public int QPackStaticTableId => GetResponseHeaderStaticTableId(_knownHeaderType);


Where/how is this used?

The pattern is copied from Http2HeadersEnumerator and will be used when implementing #33980.

src/Servers/Kestrel/Core/test/HttpResponseHeadersTests.cs

Tratcher · 2021-06-30T23:23:46Z

src/Servers/Kestrel/Core/test/PipelineExtensionTests.cs

+        [InlineData(2, 2)]
+        [InlineData(3, 2)]


We're now using GetMaxByteCount(1) to ensure we have enough space to write at least one character into the buffer, but for ASCII this returns 2 for some reason. I don't know how one char could become two ASCII bytes.

Tratcher · 2021-06-30T23:26:23Z

Rebased to resolve merge conflicts. I fixed a bug in WriteEncodedMultiWrite uncovered by the new benchmarks where it didn't ensure buffer space the first time through the loop.

src/Servers/Kestrel/Core/src/Internal/Http3/Http3HeadersEnumerator.cs

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

src/Servers/Kestrel/Core/src/Internal/Http2/HPackHeaderWriter.cs

Tratcher · 2021-07-01T19:51:13Z

/azp run

azure-pipelines · 2021-07-01T19:51:32Z

Azure Pipelines successfully started running 2 pipeline(s).

halter73 · 2021-07-02T23:04:13Z

src/Servers/Kestrel/Core/src/Internal/Http3/Http3FrameWriter.cs

                {
                    _log.QPackEncodingError(_connectionId, streamId, ex);
+                    _connectionContext.Abort(new ConnectionAbortedException(ex.Message, ex));
                    _http3Stream.Abort(new ConnectionAbortedException(ex.Message, ex), Http3ErrorCode.InternalError);


Shouldn't aborting the _connectionContext abort the stream implicitly?

@JamesNK? I wasn't sure about that part, I know state is tracked much differently from HTTP/2.

ghost · 2021-07-02T23:04:41Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

davidfowl · 2021-07-06T16:46:37Z

Please watch the benchmarks after this change makes it all the way in.

Tratcher added this to the 6.0-preview7 milestone Jun 22, 2021

Tratcher self-assigned this Jun 22, 2021

gfoidl reviewed Jun 23, 2021

View reviewed changes

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

Tratcher commented Jun 23, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.cs Outdated Show resolved Hide resolved

Pilchie added the area-runtime label Jun 23, 2021

Tratcher marked this pull request as ready for review June 25, 2021 23:15

Tratcher requested review from BrennanConroy, halter73 and Pilchie as code owners June 25, 2021 23:15

Tratcher requested a review from a team June 25, 2021 23:15

gfoidl reviewed Jun 28, 2021

View reviewed changes

Tratcher requested a review from JunTaoLuo June 28, 2021 16:51

Tratcher mentioned this pull request Jun 29, 2021

HttpResponseHeaders.CopyToFast move directly to next rather than if branching #32337

Merged

JunTaoLuo reviewed Jun 30, 2021

View reviewed changes

JunTaoLuo approved these changes Jun 30, 2021

View reviewed changes

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

src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs Outdated Show resolved Hide resolved

halter73 reviewed Jun 30, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http3/Http3FrameWriter.cs Show resolved Hide resolved

halter73 reviewed Jun 30, 2021

View reviewed changes

src/Servers/Kestrel/Core/test/HttpResponseHeadersTests.cs Show resolved Hide resolved

Tratcher added 9 commits June 30, 2021 14:45

TEMP: Add response header encoding callback

941c90c

Add trailers test

ca17faf

HTTP/2 response headers test

134458d

HTTP/1 custom header encoding

8534569

Custom encoding for known headers

a0ab85f

Validate custom encodings

1b2b675

Cleanup

d31b314

Http/2 hpack encoding

ed83a04

More validation, HTTP/2 trailers tests

a6834a6

Tratcher added 3 commits June 30, 2021 15:10

Revert stackalloc change

465249d

H/Qpack encoding errors

056d853

Fix test issues

b518b31

Tratcher force-pushed the tratcher/non-ascii branch from 06634ad to b518b31 Compare June 30, 2021 23:11

Tratcher commented Jun 30, 2021

View reviewed changes

JamesNK reviewed Jul 1, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http3/Http3HeadersEnumerator.cs Show resolved Hide resolved

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

JamesNK reviewed Jul 1, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http2/HPackHeaderWriter.cs Show resolved Hide resolved

gfoidl mentioned this pull request Jul 1, 2021

stackalloc behavior with constant size that isn't a power of 2 dotnet/runtime#55001

Closed

Tratcher added 2 commits July 1, 2021 10:13

Test fix

94d51ed

Log symmetry

0c03bf2

halter73 reviewed Jul 2, 2021

View reviewed changes

halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 2, 2021

halter73 approved these changes Jul 2, 2021

View reviewed changes

Tratcher merged commit 051aa95 into dotnet:main Jul 6, 2021

Tratcher deleted the tratcher/non-ascii branch July 6, 2021 16:44

This was referenced Jul 6, 2021

Changes dependant on targetting 6.0 microsoft/reverse-proxy#1055

Closed

Sync HPack encoding changes from aspnetcore dotnet/runtime#55288

Merged

pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 13, 2021

MihaZupan mentioned this pull request Nov 2, 2021

Document Kestrel response header encoding microsoft/reverse-proxy#1346

Closed

This was referenced Sep 17, 2022

Vectorized HttpCharacters #44041

Closed

Vectorize IndexOfAny on more than 5 chars dotnet/runtime#68328

Closed

stephentoub mentioned this pull request Nov 22, 2022

Use IndexOfAnyInRange in StripBidiControlCharacters dotnet/runtime#78658

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kestrel response header encoding #33776

Kestrel response header encoding #33776

Tratcher commented Jun 22, 2021 •

edited

Loading

Tratcher commented Jun 25, 2021

azure-pipelines bot commented Jun 25, 2021

JunTaoLuo left a comment

JunTaoLuo Jun 30, 2021

Tratcher Jun 30, 2021

JunTaoLuo left a comment

halter73 Jun 30, 2021

Tratcher Jun 30, 2021

Tratcher Jun 30, 2021

Tratcher commented Jun 30, 2021

Tratcher commented Jul 1, 2021

azure-pipelines bot commented Jul 1, 2021

halter73 Jul 2, 2021

Tratcher Jul 2, 2021 •

edited

Loading

ghost commented Jul 2, 2021

davidfowl commented Jul 6, 2021


		public Func<string, Encoding?> EncodingSelector { get; set; } = KestrelServerOptions.DefaultHeaderEncodingSelector;

		public int QPackStaticTableId => GetResponseHeaderStaticTableId(_knownHeaderType);

Kestrel response header encoding #33776

Kestrel response header encoding #33776

Conversation

Tratcher commented Jun 22, 2021 • edited Loading

Tratcher commented Jun 25, 2021

azure-pipelines bot commented Jun 25, 2021

JunTaoLuo left a comment

Choose a reason for hiding this comment

JunTaoLuo Jun 30, 2021

Choose a reason for hiding this comment

Tratcher Jun 30, 2021

Choose a reason for hiding this comment

JunTaoLuo left a comment

Choose a reason for hiding this comment

halter73 Jun 30, 2021

Choose a reason for hiding this comment

Tratcher Jun 30, 2021

Choose a reason for hiding this comment

Tratcher Jun 30, 2021

Choose a reason for hiding this comment

Tratcher commented Jun 30, 2021

Tratcher commented Jul 1, 2021

azure-pipelines bot commented Jul 1, 2021

halter73 Jul 2, 2021

Choose a reason for hiding this comment

Tratcher Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Jul 2, 2021

davidfowl commented Jul 6, 2021

Tratcher commented Jun 22, 2021 •

edited

Loading

Tratcher Jul 2, 2021 •

edited

Loading