Open ladeak opened 1 month ago
cc. @amcasey who has been in the discussions of the corresponding issue.
Thanks for this! Thorough, as always.
Some notes:
Some test questions (you may already have code/coverage for this - I haven't checked):
@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.
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:
@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.~
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.
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.
@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!
@amcasey Going to come back tomorrow with some findings.
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:
Http2
limits?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?
@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?
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).
@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.
Thanks for the prototype and the thoughtful write-up.
@amcasey , I added a commit that has a new limit on
KestrelServerLimits
, and this is also respected byHttp2FrameWriter
. 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.).
Assuming no perf impact, I would probably accept that level of complexity to get the extra protection. Having said that, it feels like there are ways we could reduce the complexity. What if we capped the header size before hpack? Would that let us do a single check up-front? Given how much larger we expect the limit to be than any app would reasonably use, I don't think hpack is going to be what saves people (i.e. by keeping them under the limit).
One implementation note: when you give people an option like this, it's not unusual for them to pass int.MaxValue
, so you have to think very carefully about any additions and ordered comparisons you do. If we keep the checks, I think they will probably need to be hardened against that.
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.
It does seem like a well-behaved server ought to respect that setting. Maybe we had a reason for not already doing so? @halter73 @JamesNK?
If we were to add that functionality (possibly in a separate PR), it would be important to ensure that it uses different error text from the internal server limit so app authors know they can't control it.
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.
I would agree that a public limit should apply to all protocols. Would the H/1.1 and H/3 changes be simpler if we used a limit on the pre-compression size?
Again, any insight from @halter73 @JamesNK on why H/1.1 doesn't already have such a limit would be welcome.
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.
I'm not sure I understand the suggestion. Isn't the buffer the thing we use for breaking the header into pieces? Does your proposed setting limit the size of each piece or is it a different buffer?
So that leads my thought process back to your initial suggestion: an appcontext switch for
MaxHeaderBufferSize
- although not sure if numbers are supported there.
It's slightly wonky. You can use Set/GetData but you get an object and then you have to check for both int and string. It's not ideal, but it's possible. I could live with making it an appcontext switch at first and eventually promoting it to a setting but, with the information I have now, I think I would still lean towards having a public setting. As always, I'm open to arguments in favor of going a different way.
There's an example here.
Assuming no perf impact, I would probably accept that level of complexity to get the extra protection. Having said that, it feels like there are ways we could reduce the complexity. What if we capped the header size before hpack? Would that let us do a single check up-front? Given how much larger we expect the limit to be than any app would reasonably use, I don't think hpack is going to be what saves people (i.e. by keeping them under the limit).
I would agree that a public limit should apply to all protocols. Would the H/1.1 and H/3 changes be simpler if we used a limit on the pre-compression size?
The reason I went the way it is implemented, because the encoding is also applied by the HPack writer, but this suggestion makes sense to investigate. I will check if there is any common place for all protocols to perform this.
One implementation note: when you give people an option like this, it's not unusual for them to pass
int.MaxValue
, so you have to think very carefully about any additions and ordered comparisons you do. If we keep the checks, I think they will probably need to be hardened against that.
Makes sense, thank you for the reminder.
I'm not sure I understand the suggestion. Isn't the buffer the thing we use for breaking the header into pieces? Does your proposed setting limit the size of each piece or is it a different buffer?
Each piece - the reason I keep coming back to this idea is because my understanding was that the problem is allocating a really large buffer, and this would be an easy check. But as discussed above, let me try to pursue validating the total headers before the write operation.
I am thinking (and moving - wip) the limit logic to HttpProtocol
after the headers collection is marked readonly. I believe it is going to be a common place for all protocol versions.
I was actually thinking of a single check for each protocol, but I'm fine with merging the checks if that's an option.
Each piece - the reason I keep coming back to this idea is because my understanding was that the problem is allocating a really large buffer, and this would be an easy check. But as discussed above, let me try to pursue validating the total headers before the write operation.
Yeah, I think I was just mixing myself up. Preventing the actual problem seems like a viable way forward. We just need to make sure we're able to give the user an intelligible setting.
I was actually thinking of a single check for each protocol, but I'm fine with merging the checks if that's an option.
In HttpProtocol
I would need to iterate the headers an additional time, so I don't think it would be viable option eventually.
What if we capped the header size before hpack?
I am still looking at this idea. If I want to avoid iterating all headers an additional time, I find it very suitable to calculate the header length in Http2FrameWriter
where the headers are iterated, and in HPackHeaderWriter
's EncodeHeadersCore method where the encoder available at hand to calculate a "more-or-less" length before applying the HPACK compression.
I will prepare a prototype of this solution. The confusing part is that EncodeHeadersCore already has an out int length
parameter, and I feel the urge to introduce a second out int rawLength
or so parameter.
Thanks! I agree that an additional iteration would be undesirable.
I'm going to be out of town next week, but @mgravell should be around to answer your questions (except Monday, which is a holiday for him).
@amcasey (and @mgravell ) I moved the size validation into HPackHeaderWriter
. It is using the payload length when available from the underlying HPack encoder, or it 'calculates' a header size disregarding static and dynamic tables. If the encoder used those tables, it would have been very likely that header still fits within the buffer anyway.
This PR is still not covering HTTP/1.1 and HTTP/3, but I remove the Draft label, so then we can discuss the other HTTP version and the new limit. For HTTP/3, I believe the same frame size issue is present, so that might worth having a look in a separate PR maybe? For HTTP/1.1 I am slightly concerned about the default value of the new limit being too aggressive.
Another consideration on the way the headers length is aggregated: because it is HTTP version specific, and because in the current HTTP/2 the implementation piggy backs onto HPack
's calculation for perf reasons, it means the shared KestrelServerLimits
will result different behavior on the different version of HTTP, given different compression/encoding/static-dynamic tables used.
So, if this approach is kept, that we have the limit calculated for each protocol (and to me it seems reasonable), maybe separate settings would make more sense? (unless it gets too granular, or these differences are acceptable)
@ladeak I still feel like I'm missing something fundamental. It seems like the input to the writer is a collection of header names and values. Is that correct? Maybe it's lazy or something? Ignoring perf for a moment, it seems like we could just enumerate the collection of headers to check whether, before compression, they are individually or collectively too long. There's a bit of uncertainty around encodings, but it seems like a similar limit could be applied regardless of the protocol - look at all the headers before trying to write them out and put a cap on the maximum length. There are lot of practical reasons why that might not be a good course of action, but I'd like to confirm I at least understand what's possible.
While I was out, I had some chance to look at the HTTP/2 spec and noticed that 8.2.3 gives us another way to split up long headers. Do you happen to know whether we're already attempting to do that?
@ladeak I still feel like I'm missing something fundamental. It seems like the input to the writer is a collection of header names and values. Is that correct? Maybe it's lazy or something? Ignoring perf for a moment, it seems like we could just enumerate the collection of headers to check whether, before compression, they are individually or collectively too long. There's a bit of uncertainty around encodings, but it seems like a similar limit could be applied regardless of the protocol - look at all the headers before trying to write them out and put a cap on the maximum length. There are lot of practical reasons why that might not be a good course of action, but I'd like to confirm I at least understand what's possible.
That was my attempt with HttpProtocol
approach, but seemed like large perf hit - and hence I abandoned. My idea there was to sum the chars in the header strings.
EDIT: to answer the question: yes, in general you are correct. We get a set of headers (HttpHeaders
), and in case of HTTP/2, Http2HeadersEnumerator
is used to enumerate the headers. Technically we could enumerate twice.
So, hosting an endpoint with Http1AndHttp2AndHttp3 with a single Kestrel limit means the following:
Not sure if there is a third option or a better way of doing things. If I understand you correctly 'similar' limit is good enough? I am little lost on which direction would you prefer.
While I was out, I had some chance to look at the HTTP/2 spec and noticed that 8.2.3 gives us another way to split up long headers. Do you happen to know whether we're already attempting to do that?
I had a look at it earlier, but then when I was looking at rfc7540 - 8.1.2.5 which directs to rfc7230 - 3.2.2 that says:
A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).
My understanding that this cannot be applied in general, only for headers that are defined as a list (if I understand this correctly)
@amcasey , 1, Does it bother you that I click resolve comment when I push an update on the given comment? (I do it only so that I can track what I changed or not). 2, What do you think about the questions posed here: https://github.com/dotnet/aspnetcore/pull/55322#issuecomment-2094145610 - Edit: I realize https://github.com/dotnet/aspnetcore/pull/55322#issuecomment-2108675120 is answering the questions probably 3, Where would you see I shall take this implementation next, which part to look into/clarify?
EDIT: to answer the question: yes, in general you are correct. We get a set of headers (HttpHeaders), and in case of HTTP/2, Http2HeadersEnumerator is used to enumerate the headers. Technically we could enumerate twice.
Thanks! I think we're on the same page now - there's a very straightforward way to check whether the headers are too long, but it would be expensive.
My understanding that this cannot be applied in general, only for headers that are defined as a list (if I understand this correctly)
Nope, it's definitely not a general solution and we don't know what problems people are hitting in practice, so we don't know if this special handling would help them. I was mostly asking out of curiosity, though it's possible that it would actually help in the target scenario (unknowable, unfortunately).
Does it bother you that I click resolve comment when I push an update on the given comment?
I looked through the ones you resolved and they all look good to me. If I ask an open-ended question, I think it's helpful to keep it around for discussion purposes, but any comment where there's an easy answer (i.e. make the change or explain why it's a bad idea / impossible) can be resolved whenever you want. Thanks for checking!
Where would you see I shall take this implementation next, which part to look into/clarify?
I'll make a separate comment for this.
Regarding the size limit, you're definitely correct that I've been hedging and not providing a clear indication of what I'd prefer - that's because I don't know. π
I talked to @JamesNK offline and I think we've been thinking about the limit in the wrong way (sorry - I hate sending people down the wrong path). The server gets to decide what (and how much) it sends in a response, so I'm not sure applying a limit makes sense, even if it's configurable. If, for some reason, a server owner wants to return a 2 GB payload in HEADERS, rather than DATA, that's their call. (The client probably won't like it, but I'm not convinced we want to stop them from trying.)
I think a better thing to think about would be what would happen if someone tried that. We've never tested that before because this limit prevented any individual header from exceeding 16KB (or whatever). So maybe what we should try next is getting rid of the limit (again, sorry) and actually sending some very large response headers. If nothing breaks, there's no need for a limit. If something does break, we may find it beneficial to add a (non-configurable) limit to allow us to detect ahead of time that it's going to break. How does that sound?
@amcasey , no need to be sorry, the plan sounds reasonable to me.
Test with very large header value
app.MapGet("/", async context =>
{
context.Response.Headers["my"] = new StringValues([new string('a', 1024 * 1024 * 1024), new string('a', 1024 * 1024 * 1024)]);
await context.Response.WriteAsync("Hello World");
});
Returns: Status: InternalServerError Version: 2.0 Encoding: utf-8 Date: Wed, 15 May 2024 20:44:42 GMT Server: Kestrel Content-Length: 0
Server logs:
fail: Microsoft.AspNetCore.Server.Kestrel[13]
Connection id "0HN3L3U8PLJNC", Request id "0HN3L3U8PLJNC:00000001": An unhandled exception was thrown by the application.
System.OutOfMemoryException: Exception of type 'System.OutOfMemoryException' was thrown.
at System.String.Ctor(Char c, Int32 count)
at Program.<>c.<<<Main>$>b__0_0>d.MoveNext() in D:\repos\aspnetcore\src\Servers\Kestrel\samples\Http2SampleApp\Program.cs:line 19
--- End of stack trace from previous location ---
at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|7_0(Endpoint endpoint, Task requestTask, ILogger logger) in D:\repos\aspnetcore\src\Http\Routing\src\EndpointMiddleware.cs:line 95
at Microsoft.WebTools.BrowserLink.Net.BrowserLinkMiddleware.InvokeAsync(HttpContext context)
at Microsoft.AspNetCore.Watch.BrowserRefresh.BrowserRefreshMiddleware.InvokeAsync(HttpContext context)
at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application) in D:\repos\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http\HttpProtocol.cs:line 676
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
Request finished HTTP/2 GET https://localhost:5001/ - 500 0 - 189.3041ms
Test with large header value
context.Response.Headers["my"] = new string('a', 512 * 1024 * 1024);
Server logs:
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
Request starting HTTP/2 GET https://localhost:5001/ - - -
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0]
Executing endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[1]
Executed endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
Request finished HTTP/2 GET https://localhost:5001/ - 200 - - 1498.1057ms
Well, it all got printed on a terminal, it was fun to watch:
It tries to get a pretty large corresponding buffer too from the ArrayBufferWriter<T>
Multiple large values:
context.Response.Headers["my"] = new StringValues([new string('a', 1000 * 1024 * 1024), new string('a', 1000 * 1024 * 1024)]);
Server logs:
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[1]
Executed endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
Request finished HTTP/2 GET https://localhost:5001/ - 200 - - 1945.1286ms
But at this point my MyHttp client broke while printing the values with OOM - maybe I can skip printing it to the terminal to stress this case further.
Interestingly it got served though.
Are there any more specific scenarios you have on mind to test? Shall I create a unit test too? Are there any other aspects I should measure?
Are there any more specific scenarios you have on mind to test? Shall I create a unit test too? Are there any other aspects I should measure?
No, that looks good to me. Maybe create a test at 1 MB or something - bigger than any reasonable header, but not so big it will slow down the tests. Thanks!
To confirm that I read correctly, it looks like it works unless you actually run out of heap on the server. Is that what you observed too? Assuming that's the case, I don't think we need to add a limit, which should simplify the change quite a bit.
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
ArrayBufferWriter<byte>
(similar to .NET runtime), 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 tellHttp2FrameWriter
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. 04. main):
Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated WriteResponseHeaders 79.64 ns 0.581 ns 0.515 ns 12,556,569.9 0.0002 - - 32 B Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated WriteResponseHeaders 77.43 ns 0.355 ns 0.315 ns 12,914,602.3 0.0002 - - 32 B After changes Removing header limits related code because it is up to the service oβ¦
Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated WriteResponseHeaders 79.30 ns 0.893 ns 0.791 ns 12,609,570.9 0.0002 - - 32 B Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated WriteResponseHeaders 76.99 ns 0.721 ns 0.602 ns 12,988,922.7 0.0002 - - 32 B Fixes #4722
@amcasey I will add the test in the coming 1-2 days. I also promised to stress the server further with a better client (so it does not break with OOM while printing the headers):
This is the server code:
context.Response.Headers["my"] = new StringValues([new string('a', 1000 * 1024 * 1024), new string('a', 1000 * 1024 * 1024), new string('a', 1000 * 1024 * 1024)]);
Server logs:
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
Request starting HTTP/2 GET https://localhost:5001/ - - -
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0]
Executing endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[1]
Executed endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
Request finished HTTP/2 GET https://localhost:5001/ - 200 - - 14161.4914ms
But it broke HttpClient
, so the client side:
Request Error System.Net.Http.HttpRequestException: An error occurred while sending the request.
---> System.IO.IOException: The request was aborted.
---> System.Net.Http.HttpRequestException: The HTTP response headers length exceeded the set limit of 2147483647 bytes.
at System.Net.Http.Http2Connection.Http2Stream.AdjustHeaderBudget(Int32 amount)
at System.Net.Http.Http2Connection.Http2Stream.OnHeader(HeaderDescriptor descriptor, ReadOnlySpan`1 value)
at System.Net.Http.Http2Connection.Http2Stream.OnHeader(ReadOnlySpan`1 name, ReadOnlySpan`1 value)
at System.Net.Http.HPack.HPackDecoder.ProcessHeaderValue(ReadOnlySpan`1 data, IHttpStreamHeadersHandler handler)
at System.Net.Http.HPack.HPackDecoder.ParseHeaderValue(ReadOnlySpan`1 data, Int32& currentIndex, IHttpStreamHeadersHandler handler)
at System.Net.Http.HPack.HPackDecoder.DecodeInternal(ReadOnlySpan`1 data, IHttpStreamHeadersHandler handler)
at System.Net.Http.Http2Connection.ProcessHeadersFrame(FrameHeader frameHeader)
at System.Net.Http.Http2Connection.ProcessIncomingFramesAsync()
--- End of inner exception stack trace ---
at System.Net.Http.Http2Connection.ThrowRequestAborted(Exception innerException)
at System.Net.Http.Http2Connection.Http2Stream.CheckResponseBodyState()
at System.Net.Http.Http2Connection.Http2Stream.TryEnsureHeaders()
at System.Net.Http.Http2Connection.Http2Stream.ReadResponseHeadersAsync(CancellationToken cancellationToken)
at System.Net.Http.Http2Connection.Http2Stream.ReadResponseHeadersAsync(CancellationToken cancellationToken)
at System.Net.Http.Http2Connection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
--- End of inner exception stack trace ---
at System.Net.Http.Http2Connection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
But it broke HttpClient , so the client side:
I'm okay with breaking HttpClient
- a server doing this for real could be working with a custom client that accepts giant headers. Thanks for confirming!
@ladeak I saw your update, but I don't think I'm going to have a chance to look at it today.
@amcasey I wish you a smooth recovery.
I'm not feeling all that well (and might be out at the beginning of next week), so I might not be thinking very clearly, but I think I need an overview of the relationships among
WriteResponseHeadersUnsynchronized
,WriteDataAndTrailersAsync
,SplitHeaderFramesToOutput
, andFinishWritingHeadersUnsynchronized
.It looks like all calls to
FinishWritingHeadersUnsynchronized
are immediately preceded by calls toSplitHeaderFramesToOutput
, but it also callsSplitHeaderFramesToOutput
? Maybe that call should be moved intoFinishWritingHeadersUnsynchronized
?
Yes, I actually used to have an additional callsite for SplitHeaderFramesToOutput
in FinishWritingHeadersUnsynchronized
. The initial implementation called an initial split before entering the loop.
I removed this initial call to SplitHeaderFramesToOutput
so FinishWritingHeadersUnsynchronized
had less branching and parameters needed. When writing the TRAILERS the SplitHeaderFramesToOutput
is not preceeded by FinishWritingHeadersUnsynchronized
, so FinishWritingHeadersUnsynchronized
method was requiring the branching operation on endOfHeaders
.
I also felt it is a bit cleaner to pass the isFramePrepared
explicitly from the same method where the frame is actually prepared rather than implicitly figuring it out in FinishWritingHeadersUnsynchronized
.
But of course, I am happy to refactor 'back' as suggested.
Why do both
WriteDataAndTrailersAsync
andFinishWritingHeadersUnsynchronized
have loops that grow the buffer even though one calls the other? Naively, it feels like exactly one method should be responsible for looping and others should delegate to it as necessary. Maybe I'm missing some functional requirement or perf benefit.
WriteDataAndTrailersAsync
calls BeginEncodeHeaders
and loops only for the first buffer
, and FinishWritingHeadersUnsynchronized
write CONTINUATIONS only.
The WriteDataAndTrailersAsync
requires the loop, so if we have a single large trailer, BeginEncodeHeaders
will return BufferTooSmall
and calling FinishWritingHeadersUnsynchronized
could result an empty HEADERS, CONTINUATION, CONTINUATION, etc. frames, because FinishWritingHeadersUnsynchronized
does not have this notion of different frame types.
Note, when writing the response headers the loop could be dropped because there is a status code, so we never end up with empty HEADERS frame.
I am looking at the difference between HPackHeaderWriter.BeginEncodeHeaders
(the overload without the status code) and HPackHeaderWriter.ContinueEncodeHeaders
, and I wonder if it can be refactored in a way to accommodate the above question - not sure if it is a good path to go down though..
@ladeak I sketched out the code organization I had in mind. It's probably wrong - I didn't even try to compile it - but I think it illustrates my two goals: making headers and trailers look similar and only manipulating the buffer size in a single method. It also restores the fast path in the trailers case - I think that may have been lost.
What do you think? I wrote it as a way to clarify my questions and not as a proposal.
Same caveat as before: I'm not feeling very well, so it may not make sense at all. π
@amcasey thank you, I see your idea now. With an initial read I see two edge cases:
BeginEncodeHeaders
calls MoveNext
. I pull the sketch later today and try to take it further.I wish you getting better. Please note, that I also have an activity for the next 5 weeks 3 days a week that slightly reduces my normal capacity (it started 2 weeks ago).
the 'Done' case when writing the HEADER frame might not enjoy the 'fast path', because the buffer could be already set to a larger capacity than the frame size when writing the response header
Probably not the only oversight. π
I would avoid allocating new buffers because the original Http2FrameWriter also only had a single allocation
Fair point, but I'm a little concerned about the buffer never shrinking (AFAICT).
there was a reason I had to re-initialize the '_headersEnumerator' for the TRAILERS
I'm interested to know what you find.
I wish you getting better
Thanks!
reduces my normal capacity
No worries. We're very grateful for the work you've already put in - do what works for you.
@amcasey I made the 'Sketch' working and passing tests on my machine. I push changes with the latest commit.
I could 'only' make it work by adding a new HPackHeaderWriter.RetryBeginEncodeHeaders
method. The other thing I notice, is that the 'Fast' path is very slightly slower, as it needs to call Advance
method and read the WrittenSpan
property.
I would add maybe 1-2 additional tests to be more comfortable with this implementation though. (ie. for the new RetryBeginEncodeHeaders
).
I have not yet handled the fact that _headerEncodingBuffer
grows within a connection. My initial idea was to address it by recreating a new one after the trailers are written and if the size is larger to the _maxFrameSize
.
Then it might work only for certain headers. ie. I have a large Cookie (when I have trouble with large headers, it is always a cookie), then that cookie would be large on all streams of the connection.
I would like to avoid allocating unnecessary though. I will come back to this tomorrow and continue.
EDIT: I could not use header's Reset because it does not reset the unKnownEnumerator's position.
EDIT2: I realize that ArrayBufferWriter
is anyway a less optimal choice, because it does a copy upon growing the underlying buffer. Instead, I will restore the original array as before and use an ArrayBufferPool
for headers that need a large buffer.
I have updated the Benchmarks, and I noticed a significant degradation on the main branch. My measurements from earlier this month vs now:
Before changes (updated 2024. 05. 04. main branch):
Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|
WriteResponseHeaders | 79.64 ns | 0.581 ns | 0.515 ns | 12,556,569.9 | 0.0002 | - | - | 32 B |
Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|
WriteResponseHeaders | 77.43 ns | 0.355 ns | 0.315 ns | 12,914,602.3 | 0.0002 | - | - | 32 B |
Before changes (updated 2024. 05. 22. main branch):
Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|
WriteResponseHeaders | 186.6 ns | 1.32 ns | 1.17 ns | 5,357,686.3 | 0.0002 | - | - | 32 B |
Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|
WriteResponseHeaders | 185.9 ns | 1.35 ns | 1.27 ns | 5,377,844.8 | 0.0002 | - | - | 32 B |
I have updated the Benchmarks, and I noticed a significant degradation on the main branch. My measurements from earlier this month vs now:
Nicer code definitely doesn't justify that kind of perf hit, but I'd be interested to know what's causing it. Maybe the slow path just needs to be in a separate helper method to make the JIT happy?
I could 'only' make it work by adding a new HPackHeaderWriter.RetryBeginEncodeHeaders method
I'm not sure I understand what's going wrong here. It sounds like maybe WriteDataAndTrailersAsync
needs to reset or re-initialize _headersEnumerator
if it sees BufferTooSmall
?
EDIT: I could not use header's Reset because it does not reset the unKnownEnumerator's position.
I think this is saying that resetting the enumerator would have been an alternative to introducing RetryBeginEncodeHeaders
, but it doesn't work with the unknown-enumerator test hook? Can we either make it work or just accept that that implementation throws? (Or maybe I've misunderstood entirely.)
EDIT2: I realize that ArrayBufferWriter is anyway a less optimal choice, because it does a copy upon growing
I like the pool approach. It seems like it also prevents the frame writer from holding onto a huge buffer for the lifetime of the connection (though I agree that that it's likely to want a big buffer for the next request).
Your changes make sense to me but I didn't read line-by-line because it seems like you're still investigating things (esp perf?). Let me know if there's something specific you want me to look at. Thanks again!
For trailer, even the first write might end up with BufferToSmall
when there is only a single large trailer value.
This means that when we repeatedly use BeginEncodeHeaders
. But BeginEncodeHeaders
moves the header enumerator's position, so that needs to be somehow reset. There are three ways I could think of resetting the enumerator to the beginning (before the first item):
FinishWritingHeadersUnsynchronized
for this seemed wrong._headersEnumerator.Reset
, but it only resets the 'known' headers not the unknown collection. Is this bug? Not sure but applies to trailers and headers both.RetryBeginEncodeHeaders
which does the same as BeginEncodeHeaders
but without moving the enumerator - I chose this option.I am currently not investigating why the main
branch shows a performance degradation, I was hoping it is already known - if not, I wanted to bring some light on it. There was only a single change in the file, but that change does not seem to be related.
@ladeak Sorry for the delay - I should have mentioned that the US had a holiday yesterday.
For trailer, even the first write might end up with BufferToSmall when there is only a single large trailer value. This means that when we repeatedly use BeginEncodeHeaders.
Yep, I understood this part. Can you tell me more about the "unknown collection"? Do you mean _genericEnumerator? It's quite possible there's an oversight in a Reset
implementation somewhere, but I'd like to make sure I understand your concern properly before I suggest anything.
I am currently not investigating why the main branch shows a performance degradation
Oops, I think I must have misread your comment. I thought you were saying that all the changes we'd made since your original PR had slowed down the new code path, but it sounds like you're saying main
got slower without any of your changes? That's quite concerning. Let me ask our perf crew before you invest more time.
Edit: sounds like we're not specifically aware of a problem in that area. Would you be willing to share your benchmark code to make it easier for us to track it back to a particular commit? Edit 2: the easiest thing might be to file a new perf issue about it, with as much detail as you're comfortable providing. If you tag me, I'll make sure the right people look at it. Thanks!
The benchmark I have been using is on existing on the main branch: Http2FrameWriterBenchmark.WriteResponseHeaders
.
For the _headersEnumerator
issue, this test captures the problem:
[Theory]
[InlineData("Server")]
[InlineData("Date")]
[InlineData("My")]
public void HeaderEnumeratorReset(string headerName)
{
var headers = new Internal.Http.HttpResponseHeaders();
headers.TryAdd(headerName, "somevalue");
var headerEnumerator = new Http2HeadersEnumerator();
headerEnumerator.Initialize(headers);
Assert.True(headerEnumerator.MoveNext());
headerEnumerator.Reset();
Assert.True(headerEnumerator.MoveNext()); // False in case of headerName 'My' but true for the others
}
@ladeak Do you have updated numbers for your change (even though the baseline has moved)? I think things are looking pretty good, assuming the fast path hasn't regressed.
@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.
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 tellHttp2FrameWriter
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:Before changes (updated 2024. 05. 31. main):
After changes 2024. 05. 31. Adjusting the logic of _headersEncodingLargeBufferSize to avoid 0 valβ¦
Fixes #4722