Allow HTTP2 encoder to split headers across frames #55322

ladeak · 2024-04-23T20:37:53Z

Allow the HTTP2 encoder to split headers across frames

Enable Kestrel's HTTP2 to split large HTTP headers into HEADER and CONTINUATION frames.

Description

Kestrel's HTTP2 implementation limits the max header size to the size of the frame size. If a header's size is larger than the frame size, it throws an exception: throw new HPackEncodingException(SR.net_http_hpack_encode_failure);. RFC 7540 allows the headers to be split into a HEADER frame and CONTINUATION frames.
Before the change Kestrel only used CONTINUATION frames when headers fitted fully within a frame.

This PR changes the above behavior by allowing to split even a single HTTP header into multiple frames. It uses an ArrayPool<byte>, to back the buffer used by the HPack encoder.

When the HPack encoder reports that a single header does not fit the available buffer, the size of the buffer is increased. Note, that the .NET runtime implementation on HttpClient writes all headers to a single buffer before pushing it onto the output, contrary to this implementation that keeps the semantics of Kestrel. It only increases the buffer when a single header fails to be written to the output, otherwise the old behavior is kept. My intention was to keep this behavior so that memory-wise it does not use more memory than the single largest header or the max frame size.
With this PR HPackHeaderWriter uses an enum to tell Http2FrameWriter to increase the buffer or not. When the buffer is too small, its size is doubled.

This behavior is also implemented for trailers. Note that in case of headers, the HEADER frame is never empty because of the response status, while this is not true for trailers. Hence there is a subtle difference when getting the buffer for the initial frame of a header vs. a trailer.

I updated existing tests asserting the previous behavior and added new tests to validate the proposed changes.

Performance

Performance-wise the change is not expected to increase throughput (given it must do more to enable this use-case) but the goal is to have the 'slow-path' is only in the case when a single header is too large. I used the existing Http2FrameWriterBenchmark to compare performance before and after:

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.22631
12th Gen Intel Core i7-1255U, 1 CPU, 12 logical and 10 physical cores
.NET SDK=9.0.100-preview.4.24218.26
  [Host]     : .NET 9.0.0 (9.0.24.21901), X64 RyuJIT
  Job-KLBPPQ : .NET 9.0.0 (9.0.24.21807), X64 RyuJIT

Before changes (updated 2024. 05. 31. main):

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	181.9 ns	0.98 ns	0.87 ns	5,498,122.8	0.0002	-	-	32 B

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	186.6 ns	1.02 ns	0.90 ns	5,359,175.4	0.0002	-	-	32 B

After changes 2024. 05. 31. Adjusting the logic of _headersEncodingLargeBufferSize to avoid 0 val…

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	184.4 ns	3.45 ns	3.38 ns	5,422,557.8	0.0002	-	-	32 B

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	186.7 ns	1.15 ns	1.08 ns	5,357,195.5	0.0002	-	-	32 B

Fixes #4722

ladeak · 2024-04-23T20:39:49Z

cc. @amcasey who has been in the discussions of the corresponding issue.

amcasey · 2024-04-24T00:07:24Z

Thanks for this! Thorough, as always.

Some notes:

The CI failure is unrelated. No point in re-running, since we're expecting revisions.
It may be a little while before I have time to review this in detail.
The description mentions doubling the size of a buffer, as needed - I assume there's a cap on the size of that buffer so it can't grow indefinitely and DoS the server?
The description (and tests) only mention spreading a header into a single CONTINUATION. I'm assuming this will work if the header is, e.g. 33 KB?
Is there a place we could use a simple flag to disable the new behavior and revert to the old behavior? e.g. if I wanted a NeverSplitHeaders appcontext switch, would that be straightforward to add or would be need to duplicate a bunch of code.

Some test questions (you may already have code/coverage for this - I haven't checked):

What happens if the header name is well-known and gets compressed? Is the 16KB limit based on the compressed or uncompressed size?
What happens if the header name itself is more than 16KB? (Throwing might be appropriate, but we should at least know.)
What happens if the header is larger than the maximum total header size? (I'm pretty sure there's an Http2ServerLimit for this.)
I noticed that HPACK had some handling for never-compressed literals. Does this work for those?
If there's a very short header before the very long header (e.g. one that compresses to one or two bytes), is that first HEADERS frame tiny?

ladeak · 2024-04-24T06:27:54Z

@amcasey let me reply inline:

The description mentions doubling the size of a buffer, as needed - I assume there's a cap on the size of that buffer so it can't grow indefinitely and DoS the server?

You are wondering about headers that are roundtripped to the client with large size (previously would fail, now it could DDoS)? I need to check Kestrel's H2 header size limits (you also mention), but there is nothing in the Http2FrameWriter in this regard.

The description (and tests) only mention spreading a header into a single CONTINUATION. I'm assuming this will work if the header is, e.g. 33 KB?

It can span into zero or more CONTINUATION frames.

Is there a place we could use a simple flag to disable the new behavior and revert to the old behavior? e.g. if I wanted a NeverSplitHeaders appcontext switch, would that be straightforward to add or would be need to duplicate a bunch of code.

There is no such place, but it could be very well built along the Kestrel's limit or an AppContext switch. Please let me know if building such a would be preference. But note, that previously "possible" use-cases still work the same as before, so the switch would only control if large headers are allowed or not -> hence a limit might be suitable option.

What happens if the header name is well-known and gets compressed? Is the 16KB limit based on the compressed or uncompressed size?

I did not come across compression/no-compression on this path. HPack encodes the header values into this buffer.

What happens if the header name itself is more than 16KB? (Throwing might be appropriate, but we should at least know.)

The header is written to a buffer, which is split into CONTINUATION frames, so it does not matter if the name or the value is being oversized.

What happens if the header is larger than the maximum total header size? (I'm pretty sure there's an Http2ServerLimit for this.)

MaxRequestHeaderFieldSize ? -> I need to test the behavior.

I noticed that HPACK had some handling for never-compressed literals. Does this work for those?

It works on anything that HPack writes to my understanding. I will double confirm.

If there's a very short header before the very long header (e.g. one that compresses to one or two bytes), is that first HEADERS frame tiny?

-> If the long one does not fit in the same frame, yes, the initial header will be sent in a tiny frame. This is even true for the "current" behavior.

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

src/Servers/Kestrel/shared/HPackHeaderWriter.cs

JamesNK · 2024-04-24T07:28:36Z

Background: I've looked at HTTP2 headers a lot. I wrote some of the dynamic support and rewrote the writer and parser at one point.

I haven't looked through the code in detail yet. Some initial thoughts:

Well done for jumping into HTTP2. It's complex. And Kestrel's implementation is complex.
I like the feature. Kestrel's header limit on frame size has always felt arbitrary. We did it because it was easy and fast, not because it was the best thing to do. I recently wrote a feature that put a lot of content into a header and was mindful of this limitation.
I know Kestrel has various limits for request headers. I don't believe there are limits on response headers (other than the max frame size). The idea is an app is responsible for sending the response headers so no need to limit it. However, we should look to see what HTTP/1.1 does in this area. If it doesn't limit what a response can do with headers, then neither should HTTP/2.
But we should stress this and put some kind of upper limit. What happens if someone wants a 20-megabyte response header? Or a 2-gigabyte response header? Something should blow up before the server tries to double a gigabyte buffer and blows up from an excessive amount of data.
Reading and writing response headers is extremely performance critical. Because it's stateful (the dynamic table is on the connection) it basically locks everything. Must reduce any performance overhead.

amcasey · 2024-04-24T23:56:24Z

@ladeak Thanks! Your responses make sense.

Regarding the appcontext switch, I regard this as a fairly risky change because it's touching such critical code (not because of anything you've done - just the circumstances). It would be good to at least think about how to make it possible to revert (as exactly as possible) to the old behavior. It may turn out to require too much duplicate code or API changes or something that makes it unacceptable, but I think we need to at least know what it would take. Open to differing opinions from @mgravell or @JamesNK.

JamesNK · 2024-04-25T00:00:09Z

I don't think we need a switch. If this lands in a mid .NET 9 preview, then it should go through a lot of use and testing before GA.

For example, the work David did to rewrite response writing in .NET 7(?) was much more complex and we didn't have a fallback.

amcasey · 2024-04-25T00:15:47Z

I don't think we need a switch. If this lands in a mid .NET 9 preview, then it should go through a lot of use and testing before GA.

For example, the work David did to rewrite response writing in .NET 7(?) was much more complex and we didn't have a fallback.

Good enough for me. I hadn't considered how well exercised this code is by TechEmpower, etc.

amcasey · 2024-04-26T21:50:21Z

@ladeak Did you receive the initial feedback you needed? Is this ready for a full review or are you still working on it. There's no rush - I just wondered whether the next steps were our responsibility. Thanks!

ladeak · 2024-04-26T21:53:20Z

@amcasey Going to come back tomorrow with some findings.

ladeak · 2024-04-27T16:08:07Z

Discussion about the header size limits: as I understood there is a general desire to have a limit. However, response headers mostly depend on the app, and the way app handles headers. I have not found limits for HTTP/1.1. An empty/default Kestrel ASP.NET Core webapp also allows as large headers as desired with h1. On the consumer-side I ran into limits though (H1):

.NET HttpClient has MaxResponseHeadersLength with the default of 65536 bytes.
curl on Linux - curl: (27) Rejected 140725 bytes header (max is 102400)!
curl on Windows same as on Linux - curl: (27) Out of memory
Chromium - Edge: returns ERR_RESPONSE_HEADERS_TOO_BIG at ~262000 bytes.

When I run the app with IISExpress, it seems to only returns the remainder of 64k. (header mod 65536).

@amcasey , the following questions would need further clarification:

hardcode a "big enough" limit or to expose this on Kestrel options under the Http2 limits?
what default value should this limit have?

amcasey · 2024-04-29T19:42:52Z

hardcode a "big enough" limit or to expose this on Kestrel options under the Http2 limits?
what default value should this limit have?

Since the spec doesn't give a limit, I think we need to give users a way to override whatever we decide. I'm not sure why it would be specific to http/2 though - presumably the same concern applies to http/1.1 or http/3.

My first thought for a default would be double MaxRequestHeadersTotalSize (i.e. 64 BK), but @JamesNK makes the sensible point that it's really at the server's discretion and we really just need a cap that prevents things from getting out of hand - maybe 1 MB?

If we were to decide this shouldn't get a public API, I'd want to go even higher - maybe 10 MB.

Thoughts, @JamesNK @mgravell?

ladeak · 2024-04-29T21:15:10Z

@amcasey I think I have not thought about http/1.1 about a limit, given it does not have currently, and setting 64KB would be breaking, wouldn't it? (I am not sure how difficult it could be to implement this for http/1.1, http/3 looks similar to h2 in code structure. But it makes sense from the point of view you describe that it could be a setting that applies to all versions of http.

A question if it is public: should it apply to a single header or to the total headers. Consumers HttpClient and Edge had a total while curl per header limit.
If it is total headers, does it include trailers?

amcasey · 2024-04-29T22:14:47Z

A question if it is public: should it apply to a single header or to the total headers. Consumers HttpClient and Edge had a total while curl per header limit.
If it is total headers, does it include trailers?

Because we're guarding against resource utilization rather than malformed responses, I think the limit should apply to the total size of all headers, rather than to the size of any individual header. Similarly, if we're reducing the whole detection mechanism to a single number, I would expect trailers to be included. I'm open to feedback on both.

I think I have not thought about http/1.1 about a limit, given it does not have currently, and setting 64KB would be breaking, wouldn't it?

Yes, it would. I think we generally accept breaks in service of DoS prevention, but I agree that this is a strong argument for choosing a default that is larger than we expect anyone to use in practice.

If we felt really strongly about this, I could live with adding limits to both http/2 and http/3 and not to KestrelServerLimits directly. I just don't want to end up in a state where we have three identical properties (or more, when http/4 comes out).

ladeak · 2024-05-01T16:42:21Z

@amcasey , I added a commit that has a new limit on KestrelServerLimits, and this is also respected by Http2FrameWriter.
I think the code ended up quite a bit complicated (enforcing the limit for written headers + headers not yet written but requiring a larger buffer, etc.).

One thing I found on the way: SETTINGS_MAX_HEADER_LIST_SIZE is an advisory setting, now with the limit might worth also respecting it. Not all clients send it (for example, HttpClient does not) - unfortunately it is not sufficient against DoS.

I would expect similar implementation would be needed on H/1.1 and H/3 (because the public setting is on Kestrel level), hence not sure if a public limit is worth all the complexity to be added.

Maybe a setting called MaxHeaderBufferSize would simplify things a lot, but I could also see why you would not want to expose such an implementation detail related setting on a public API. So that leads my thought process back to your initial suggestion: an appcontext switch for MaxHeaderBufferSize - although not sure if numbers are supported there.

…ncodingLargeBufferSize

…ues.

ladeak · 2024-05-31T19:09:36Z

@amcasey I have updated the performance measurements after the latest commit.
This only measures the "fast-path". We could measure the slow-path as well, but there is no base-case for that as it has thrown previously.

amcasey · 2024-05-31T19:25:40Z

@amcasey I have updated the performance measurements after the latest commit. This only measures the "fast-path". We could measure the slow-path as well, but there is no base-case for that as it has thrown previously.

Thanks! It's hard to measure perf without a baseline, but it might be interesting to compare a 20 KB header with the same header split in two (i.e. two 10 KB headers).

ladeak · 2024-05-31T20:04:43Z

@amcasey I have extended the corresponding benchmark.

BEFORE (LargeHeaderSize is in KB):

Method	LargeHeaderSize	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	0	181.3 ns	3.37 ns	2.99 ns	5,514,739.3	0.0002	-	-	32 B
WriteResponseHeaders	10	623.4 ns	9.21 ns	8.61 ns	1,603,995.2	-	-	-	32 B

BEFORE with 2*10KB header

Method	LargeHeaderSize	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	2x10	1.163 us	0.0232 us	0.0332 us	859,808.2	-	-	-	32 B

AFTER (LargeHeaderSize is in KB):

Method	LargeHeaderSize	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
WriteResponseHeaders	0	185.5 ns	2.20 ns	1.95 ns	5,390,586.0	0.0002	-	-	32 B
WriteResponseHeaders	10	594.7 ns	10.64 ns	9.43 ns	1,681,421.4	-	-	-	32 B
WriteResponseHeaders	20	1,316.4 ns	14.32 ns	13.39 ns	759,640.1	-	-	-	32 B

amcasey · 2024-05-31T20:37:47Z

@amcasey I have extended the corresponding benchmark.

Looks like it's only about 10% slower than writing multiple smaller headers. Very cool!

amcasey · 2024-05-31T20:38:48Z

(Obviously, the CI failure wasn't you.)

…nstead.

JamesNK

Looks good. While playing around with the code I added some tests. I pushed those to your branch.

In tests you've added, is there a reason why you're merging header/continuation frames into one buffer instead of reading payloads from frames individually? Reading payloads as they arrive is little more real world and lets you test exactly headers what each frame contains.

Some minor things:

JamesNK · 2024-06-03T02:29:01Z

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

+                }
+                else
+                {
+                    _headersEncodingLargeBufferSize = checked(_headersEncodingLargeBufferSize * HeaderBufferSizeMultiplier);


_headersEncodingLargeBufferSize is updated to be bigger when required, but I don't see anywhere that resets it back it a smaller size.

For example, request 1 has a response header that is 10megs in size, so this field and the buffer eventually grows to that size. Then request 2 has a response header that is 100kb in size (larger than max frame size) but the writer rents a 10meg buffer because that is _headersEncodingLargeBufferSize size from last time.

Is this behavior intentional? Is the size preserved to avoid retrying because a connection keeps sending large response headers? Should add a comment if that is the case.

If the behavior isn't intentional, then I believe the field is only used inside FinishWritingHeadersUnsynchronized. It could be changed to a local variable.

This behavior was intentional, exactly as you said: it might be ie. a cookie that is sent on all streams and that is always large. If you think that is not a desired behavior, I am happy to make it local. In the meantime, I will leave a comment explaining this.

@JamesNK There's some discussion here: #55322 (comment)

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

JamesNK · 2024-06-03T05:30:26Z

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

+                {
+                    _headersEncodingLargeBufferSize = checked(_headersEncodingLargeBufferSize * HeaderBufferSizeMultiplier);
+                }
+                ArrayPool<byte>.Shared.Return(largeHeaderBuffer);


Should returning the buffer be in a finally to ensure its returned when an error occurs? There are definitely ways to throw when encoding headers.

My understanding was that in these cases the buffer would be garbage collected.

That's my default inclination as well, but I understood we (dotnet? aspnetcore?) had guidelines saying rented objects should not be returned in the event of an exception.

Let me know, I am happy to change already prepared on my machine.
I used to see try-finally all over for ArrayPool, but lately I see less and less.

Yes, there's definitely strong precedent for only recycling on success, especially when async operations are possible; as a side-effect this also avoids the need for some try. There might be a caveat here if we expect failure in a significant % (exceptions as flow control), but I don't think that applies here

JamesNK · 2024-06-03T05:31:26Z

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

+        }
+        if (largeHeaderBuffer != null)
+        {
+            ArrayPool<byte>.Shared.Return(largeHeaderBuffer);


Should this be in a finally?

My understanding was that in these cases the buffer would be garbage collected.

Not returning doesn't create a memory leak, but it could cause future rent calls to allocate because buffers haven't been returned.

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

src/Servers/Kestrel/shared/HPackHeaderWriter.cs

Co-authored-by: James Newton-King <[email protected]>

… on size increase

amcasey · 2024-06-03T16:17:44Z

As usual, the CI failures are unrelated.

mgravell

LGTM 👍 - great conversation on this one

amcasey · 2024-06-05T00:40:02Z

Since James and Marc are happy, I'll go over this one more time tomorrow and then we should be good to go. Unless of course there are things you're still exploring, @ladeak?

ladeak · 2024-06-05T05:32:07Z

I have explored everything I had on mind @amcasey

amcasey

Now that we're at the end, I'm being a bit nitpicky, but it generally LGTM.

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

amcasey · 2024-06-06T01:20:01Z

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

@@ -566,32 +572,107 @@ private ValueTask<FlushResult> WriteDataAndTrailersAsync(Http2Stream stream, in
        }
    }

-    private void FinishWritingHeadersUnsynchronized(int streamId, int payloadLength, bool done)
+    private void SplitHeaderFramesToOutput(int streamId, ReadOnlySpan<byte> dataToFrame, bool endOfHeaders, bool isFramePrepared)


Personally, I might call this something like "SplitHeaderAcrossFrames" or "WriteHeaderAsMultipleFrames". Basically, I think the name should convey that a single header will be written in multiple parts (assuming I've understood the method correctly).

I think there are cases where this method is called with header data that doesn't need to be split.

Yes, such can case exists with the regular "MoreHeaders" result. I don't have a better name in mind, so I would go with the suggested SplitHeaderAcrossFrames.

IMO: Method name WriteHeaderDataAcrossFrames + comment:

Header data is written across 1-to-n frames, based on the header data size and max frame size.

amcasey · 2024-06-06T01:22:13Z

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

+                {
+                    // The size is backed in a local field and not reset for further streams on the connections.
+                    // When a large header is repeatedly returned on a connection, the buffer size won't need to be repeatedly increased.
+                    _headersEncodingLargeBufferSize = checked(_headersEncodingLargeBufferSize * HeaderBufferSizeMultiplier);


This will throw when the header is at least maxint / 2 bytes? What does it do when it throws? I remember there was an earlier fix to make sure an error didn't result in an infinite loop of empty writes. I'm pretty sure this won't cause that, but I thought I should confirm.

Note that the header may be actually already larger to int.MaxValue / 2. For example a client updates the _maxFrameSize to a custom value and we end up here with (2/3) * int.MaxValue Of course the next increase would throw, but to be clear, it does not limit the max response header size.

When it throws there, this is what it would throw:

info: Microsoft.AspNetCore.Server.Kestrel.Http2[38] Connection id "0HN45U6KT28C4": HPACK encoding error while encoding headers for stream ID 1. System.OverflowException: Arithmetic operation resulted in an overflow. at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.FinishWritingHeadersUnsynchronized(Int32 streamId, Int32 payloadLength, HeaderWriteResult writeResult) in D:\repos\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http2\Http2FrameWriter.cs:line 660 at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.WriteResponseHeadersUnsynchronized(Int32 streamId, Int32 statusCode, Http2HeadersFrameFlags headerFrameFlags, HttpResponseHeaders headers) in D:\repos\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http2\Http2FrameWriter.cs:line 523

which then ends up handled in WriteResponseHeadersUnsynchronized with a ConnectionAbortedException

But I could only get here by forcing a large initial value to _headersEncodingLargeBufferSize because in practise my machine throws an OOM way earlier, already when I try to allocate the string value of such large header.

src/Servers/Kestrel/shared/HPackHeaderWriter.cs

Co-authored-by: Andrew Casey <[email protected]>

- Rename splitting method to SplitHeaderAcrossFrames

…Headers

src/Servers/Kestrel/shared/HPackHeaderWriter.cs

Co-authored-by: James Newton-King <[email protected]>

amcasey

Woohoo!

amcasey · 2024-06-06T15:56:00Z

Thanks again for all your hard work, @ladeak!

ladeak requested review from halter73, BrennanConroy, JamesNK and mgravell as code owners April 23, 2024 20:37

dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 23, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 23, 2024

ladeak marked this pull request as draft April 23, 2024 20:39

ladeak force-pushed the ladeak-4722 branch from e7e932d to e4f0d54 Compare April 23, 2024 20:57

mkArtakMSFT assigned adityamandaleeka Apr 23, 2024

mgravell reviewed Apr 24, 2024

View reviewed changes

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

mgravell reviewed Apr 24, 2024

View reviewed changes

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

mgravell reviewed Apr 24, 2024

View reviewed changes

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

mgravell reviewed Apr 24, 2024

View reviewed changes

src/Servers/Kestrel/shared/HPackHeaderWriter.cs Show resolved Hide resolved

mgravell reviewed Apr 24, 2024

View reviewed changes

src/Servers/Kestrel/shared/HPackHeaderWriter.cs Show resolved Hide resolved

This comment was marked as spam.

Sign in to view

ladeak force-pushed the ladeak-4722 branch from 9cd0b87 to a916768 Compare May 1, 2024 16:57

ladeak added 2 commits May 31, 2024 20:47

Adjusting the logic of UpdateMaxFrameSize for setting a new _headersE…

9857dcb

…ncodingLargeBufferSize

Adjusting the logic of _headersEncodingLargeBufferSize to avoid 0 val…

95c9553

…ues.

Extenting Benachmark with headers sized 0, 10KB and 20KB

c4b93d6

ladeak and others added 2 commits June 1, 2024 09:32

Remove int.Max on header size increase and using checked arithmetic i…

3fe0a73

…nstead.

Tests

0dfc97b

JamesNK reviewed Jun 3, 2024

View reviewed changes

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

79be13b

Co-authored-by: James Newton-King <[email protected]>

build-analysis bot mentioned this pull request Jun 3, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

Review feedback: HeaderWriteResult not an inner type, adding comments…

8773062

… on size increase

mgravell reviewed Jun 3, 2024

View reviewed changes

JamesNK approved these changes Jun 5, 2024

View reviewed changes

amcasey reviewed Jun 6, 2024

View reviewed changes

ladeak and others added 3 commits June 6, 2024 08:03

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

a98500b

Co-authored-by: Andrew Casey <[email protected]>

- Moving comments around for _headersEncodingLargeBufferSize

9014f28

- Rename splitting method to SplitHeaderAcrossFrames

Merging the implementation of BeginEncodeHeaders and RetryBeginEncode…

957dfc3

…Headers

JamesNK reviewed Jun 6, 2024

View reviewed changes

Apply suggestions from code review

57cc264

Co-authored-by: James Newton-King <[email protected]>

amcasey approved these changes Jun 6, 2024

View reviewed changes

amcasey merged commit 73067c1 into dotnet:main Jun 6, 2024
26 checks passed

dotnet-policy-service bot added this to the 9.0-preview6 milestone Jun 6, 2024

Allow HTTP2 encoder to split headers across frames #55322

Allow HTTP2 encoder to split headers across frames #55322

Conversation

ladeak commented Apr 23, 2024 • edited Loading

Allow the HTTP2 encoder to split headers across frames

Description

Performance

ladeak commented Apr 23, 2024

amcasey commented Apr 24, 2024

ladeak commented Apr 24, 2024 • edited Loading

JamesNK commented Apr 24, 2024

amcasey commented Apr 24, 2024 • edited Loading

JamesNK commented Apr 25, 2024

amcasey commented Apr 25, 2024

amcasey commented Apr 26, 2024

ladeak commented Apr 26, 2024

ladeak commented Apr 27, 2024 • edited Loading

This comment was marked as spam.

amcasey commented Apr 29, 2024

ladeak commented Apr 29, 2024

amcasey commented Apr 29, 2024

ladeak commented May 1, 2024 • edited Loading

ladeak commented May 31, 2024

amcasey commented May 31, 2024

ladeak commented May 31, 2024 • edited Loading

amcasey commented May 31, 2024

amcasey commented May 31, 2024

JamesNK left a comment • edited Loading

Choose a reason for hiding this comment

JamesNK Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladeak Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladeak Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladeak Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Jun 3, 2024

mgravell left a comment

Choose a reason for hiding this comment

amcasey commented Jun 5, 2024

ladeak commented Jun 5, 2024

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

amcasey commented Jun 6, 2024

ladeak commented Apr 23, 2024 •

edited

Loading

ladeak commented Apr 24, 2024 •

edited

Loading

amcasey commented Apr 24, 2024 •

edited

Loading

ladeak commented Apr 27, 2024 •

edited

Loading

ladeak commented May 1, 2024 •

edited

Loading

ladeak commented May 31, 2024 •

edited

Loading

JamesNK left a comment •

edited

Loading

JamesNK Jun 3, 2024 •

edited

Loading

ladeak Jun 3, 2024 •

edited

Loading

ladeak Jun 3, 2024 •

edited

Loading

ladeak Jun 3, 2024 •

edited

Loading

JamesNK Jun 6, 2024 •

edited

Loading