Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow HTTP2 encoder to split headers across frames #55322

Merged
merged 27 commits into from
Jun 6, 2024
Merged

Allow HTTP2 encoder to split headers across frames #55322

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a10d9ee
Initial implementation
Apr 20, 2024
5cad996
Review feedback
Apr 25, 2024
f53dd7c
Updating a comment
Apr 25, 2024
231c716
Minor perf opt when all headers fit in a single header frame.
Apr 27, 2024
887f505
Adding MaxResponseHeadersLimit to H2 framewriter
May 1, 2024
b91a34c
Protect against overflow
May 2, 2024
a655a38
Validating the header length in HPackHeaderWriter
May 3, 2024
f31c8c8
Removing a test file commented out and adding more tests.
May 4, 2024
6ab7271
Review feedback
May 13, 2024
e21620a
Removing header limits related code because it is up to the service o…
May 15, 2024
9720c3a
A test for 1MB header value
May 16, 2024
f8f8ff9
Review comments - removing an unneeded if condition
May 18, 2024
994b27d
Sketch
May 21, 2024
a77a930
using arraypool instead of arraybufferwriter
May 22, 2024
4f14b0c
Re-using the rented buffers
May 24, 2024
46be03f
Preserving the size of the temporary larger buffer within the framewr…
May 30, 2024
9857dcb
Adjusting the logic of UpdateMaxFrameSize for setting a new _headersE…
May 31, 2024
95c9553
Adjusting the logic of _headersEncodingLargeBufferSize to avoid 0 val…
May 31, 2024
c4b93d6
Extenting Benachmark with headers sized 0, 10KB and 20KB
May 31, 2024
3fe0a73
Remove int.Max on header size increase and using checked arithmetic i…
Jun 1, 2024
0dfc97b
Tests
JamesNK Jun 3, 2024
79be13b
Update src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs
ladeak Jun 3, 2024
8773062
Review feedback: HeaderWriteResult not an inner type, adding comments…
Jun 3, 2024
a98500b
Update src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs
ladeak Jun 6, 2024
9014f28
- Moving comments around for _headersEncodingLargeBufferSize
Jun 6, 2024
957dfc3
Merging the implementation of BeginEncodeHeaders and RetryBeginEncode…
Jun 6, 2024
57cc264
Apply suggestions from code review
ladeak Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,9 @@ private Task ProcessSettingsFrameAsync(in ReadOnlySequence<byte> payload)
if (_clientSettings.MaxFrameSize != previousMaxFrameSize)
{
// Don't let the client choose an arbitrarily large size, this will be used for response buffers.
_frameWriter.UpdateMaxFrameSize(Math.Min(_clientSettings.MaxFrameSize, _serverSettings.MaxFrameSize));
// Safe cast, MaxFrameSize is limited to 2^24-1 bytes by the protocol and by Http2PeerSettings.
// Ref: https://datatracker.ietf.org/doc/html/rfc7540#section-4.2
_frameWriter.UpdateMaxFrameSize((int)Math.Min(_clientSettings.MaxFrameSize, _serverSettings.MaxFrameSize));
}

// This difference can be negative.
Expand Down Expand Up @@ -1829,4 +1831,4 @@ private static class GracefulCloseInitiator
public const int Server = 1;
public const int Client = 2;
}
}
}
135 changes: 107 additions & 28 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ internal sealed class Http2FrameWriter
/// TODO (https://github.com/dotnet/aspnetcore/issues/51309): eliminate this limit.
private const string MaximumFlowControlQueueSizeProperty = "Microsoft.AspNetCore.Server.Kestrel.Http2.MaxConnectionFlowControlQueueSize";

private const int HeaderBufferSizeMultiplier = 2;

private static readonly int? AppContextMaximumFlowControlQueueSize = GetAppContextMaximumFlowControlQueueSize();

private static int? GetAppContextMaximumFlowControlQueueSize()
Expand Down Expand Up @@ -71,8 +73,12 @@ internal sealed class Http2FrameWriter
// This is only set to true by tests.
private readonly bool _scheduleInline;

private uint _maxFrameSize = Http2PeerSettings.MinAllowedMaxFrameSize;
private int _maxFrameSize = Http2PeerSettings.MinAllowedMaxFrameSize;
private byte[] _headerEncodingBuffer;

// Keep track of the high-water mark of _headerEncodingBuffer's size so we don't have to grow
// through intermediate sizes repeatedly.
private int _headersEncodingLargeBufferSize = Http2PeerSettings.MinAllowedMaxFrameSize * HeaderBufferSizeMultiplier;
ladeak marked this conversation as resolved.
Show resolved Hide resolved
private long _unflushedBytes;

private bool _completed;
Expand Down Expand Up @@ -110,7 +116,6 @@ public Http2FrameWriter(
_headerEncodingBuffer = new byte[_maxFrameSize];

_scheduleInline = serviceContext.Scheduler == PipeScheduler.Inline;

_hpackEncoder = new DynamicHPackEncoder(serviceContext.ServerOptions.AllowResponseHeaderCompression);

_maximumFlowControlQueueSize = AppContextMaximumFlowControlQueueSize is null
Expand Down Expand Up @@ -367,12 +372,15 @@ public void UpdateMaxHeaderTableSize(uint maxHeaderTableSize)
}
}

public void UpdateMaxFrameSize(uint maxFrameSize)
public void UpdateMaxFrameSize(int maxFrameSize)
{
lock (_writeLock)
{
if (_maxFrameSize != maxFrameSize)
{
// Safe multiply, MaxFrameSize is limited to 2^24-1 bytes by the protocol and by Http2PeerSettings.
// Ref: https://datatracker.ietf.org/doc/html/rfc7540#section-4.2
_headersEncodingLargeBufferSize = int.Max(_headersEncodingLargeBufferSize, maxFrameSize * HeaderBufferSizeMultiplier);
_maxFrameSize = maxFrameSize;
_headerEncodingBuffer = new byte[_maxFrameSize];
ladeak marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down Expand Up @@ -507,11 +515,12 @@ private void WriteResponseHeadersUnsynchronized(int streamId, int statusCode, Ht
{
try
{
// In the case of the headers, there is always a status header to be returned, so BeginEncodeHeaders will not return BufferTooSmall.
_headersEnumerator.Initialize(headers);
_outgoingFrame.PrepareHeaders(headerFrameFlags, streamId);
var buffer = _headerEncodingBuffer.AsSpan();
var done = HPackHeaderWriter.BeginEncodeHeaders(statusCode, _hpackEncoder, _headersEnumerator, buffer, out var payloadLength);
FinishWritingHeadersUnsynchronized(streamId, payloadLength, done);
var writeResult = HPackHeaderWriter.BeginEncodeHeaders(statusCode, _hpackEncoder, _headersEnumerator, _headerEncodingBuffer, out var payloadLength);
Debug.Assert(writeResult != HeaderWriteResult.BufferTooSmall, "This always writes the status as the first header, and it should never be an over the buffer size.");
FinishWritingHeadersUnsynchronized(streamId, payloadLength, writeResult);
}
// Any exception from the HPack encoder can leave the dynamic table in a corrupt state.
// Since we allow custom header encoders we don't know what type of exceptions to expect.
Expand Down Expand Up @@ -548,11 +557,11 @@ private ValueTask<FlushResult> WriteDataAndTrailersAsync(Http2Stream stream, in

try
{
_headersEnumerator.Initialize(headers);
// In the case of the trailers, there is no status header to be written, so even the first call to BeginEncodeHeaders can return BufferTooSmall.
_outgoingFrame.PrepareHeaders(Http2HeadersFrameFlags.END_STREAM, streamId);
var buffer = _headerEncodingBuffer.AsSpan();
var done = HPackHeaderWriter.BeginEncodeHeaders(_hpackEncoder, _headersEnumerator, buffer, out var payloadLength);
FinishWritingHeadersUnsynchronized(streamId, payloadLength, done);
_headersEnumerator.Initialize(headers);
var writeResult = HPackHeaderWriter.BeginEncodeHeaders(_hpackEncoder, _headersEnumerator, _headerEncodingBuffer, out var payloadLength);
FinishWritingHeadersUnsynchronized(streamId, payloadLength, writeResult);
}
// Any exception from the HPack encoder can leave the dynamic table in a corrupt state.
// Since we allow custom header encoders we don't know what type of exceptions to expect.
Expand All @@ -566,32 +575,102 @@ private ValueTask<FlushResult> WriteDataAndTrailersAsync(Http2Stream stream, in
}
}

private void FinishWritingHeadersUnsynchronized(int streamId, int payloadLength, bool done)
private void SplitHeaderAcrossFrames(int streamId, ReadOnlySpan<byte> dataToFrame, bool endOfHeaders, bool isFramePrepared)
{
var buffer = _headerEncodingBuffer.AsSpan();
_outgoingFrame.PayloadLength = payloadLength;
if (done)
var shouldPrepareFrame = !isFramePrepared;
while (dataToFrame.Length > 0)
{
_outgoingFrame.HeadersFlags |= Http2HeadersFrameFlags.END_HEADERS;
}
if (shouldPrepareFrame)
{
_outgoingFrame.PrepareContinuation(Http2ContinuationFrameFlags.NONE, streamId);
}

WriteHeaderUnsynchronized();
_outputWriter.Write(buffer.Slice(0, payloadLength));
// Should prepare continuation frames.
shouldPrepareFrame = true;
var currentSize = Math.Min(dataToFrame.Length, _maxFrameSize);
_outgoingFrame.PayloadLength = currentSize;
if (endOfHeaders && dataToFrame.Length == currentSize)
{
_outgoingFrame.HeadersFlags |= Http2HeadersFrameFlags.END_HEADERS;
}

while (!done)
{
_outgoingFrame.PrepareContinuation(Http2ContinuationFrameFlags.NONE, streamId);
WriteHeaderUnsynchronized();
_outputWriter.Write(dataToFrame[..currentSize]);
dataToFrame = dataToFrame.Slice(currentSize);
}
}

done = HPackHeaderWriter.ContinueEncodeHeaders(_hpackEncoder, _headersEnumerator, buffer, out payloadLength);
private void FinishWritingHeadersUnsynchronized(int streamId, int payloadLength, HeaderWriteResult writeResult)
{
Debug.Assert(payloadLength <= _maxFrameSize, "The initial payload lengths is written to _headerEncodingBuffer with size of _maxFrameSize");
byte[]? largeHeaderBuffer = null;
Span<byte> buffer;
if (writeResult == HeaderWriteResult.Done)
{
// Fast path, only a single HEADER frame.
_outgoingFrame.PayloadLength = payloadLength;

if (done)
_outgoingFrame.HeadersFlags |= Http2HeadersFrameFlags.END_HEADERS;
WriteHeaderUnsynchronized();
_outputWriter.Write(_headerEncodingBuffer.AsSpan(0, payloadLength));
return;
}
else if (writeResult == HeaderWriteResult.MoreHeaders)
{
_outgoingFrame.PayloadLength = payloadLength;
WriteHeaderUnsynchronized();
_outputWriter.Write(_headerEncodingBuffer.AsSpan(0, payloadLength));
}
else
{
// This may happen in case of the TRAILERS after the initial encode operation.
// The _maxFrameSize sized _headerEncodingBuffer was too small.
while (writeResult == HeaderWriteResult.BufferTooSmall)
{
Debug.Assert(payloadLength == 0, "Payload written even though buffer is too small");
largeHeaderBuffer = ArrayPool<byte>.Shared.Rent(_headersEncodingLargeBufferSize);
buffer = largeHeaderBuffer.AsSpan(0, _headersEncodingLargeBufferSize);
writeResult = HPackHeaderWriter.RetryBeginEncodeHeaders(_hpackEncoder, _headersEnumerator, buffer, out payloadLength);
if (writeResult != HeaderWriteResult.BufferTooSmall)
{
SplitHeaderAcrossFrames(streamId, buffer[..payloadLength], endOfHeaders: writeResult == HeaderWriteResult.Done, isFramePrepared: true);
}
else
{
_headersEncodingLargeBufferSize = checked(_headersEncodingLargeBufferSize * HeaderBufferSizeMultiplier);
Copy link
Member

@JamesNK JamesNK Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
ArrayPool<byte>.Shared.Return(largeHeaderBuffer);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ladeak ladeak Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ladeak ladeak Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

largeHeaderBuffer = null;
}
if (writeResult == HeaderWriteResult.Done)
{
ladeak marked this conversation as resolved.
Show resolved Hide resolved
_outgoingFrame.ContinuationFlags = Http2ContinuationFrameFlags.END_HEADERS;
return;
}
}

WriteHeaderUnsynchronized();
_outputWriter.Write(buffer.Slice(0, payloadLength));
// HEADERS and zero or more CONTINUATIONS sent - all subsequent frames are (unprepared) CONTINUATIONs
buffer = _headerEncodingBuffer;
while (writeResult != HeaderWriteResult.Done)
{
writeResult = HPackHeaderWriter.ContinueEncodeHeaders(_hpackEncoder, _headersEnumerator, buffer, out payloadLength);
if (writeResult == HeaderWriteResult.BufferTooSmall)
{
if (largeHeaderBuffer != null)
{
ArrayPool<byte>.Shared.Return(largeHeaderBuffer);
_headersEncodingLargeBufferSize = checked(_headersEncodingLargeBufferSize * HeaderBufferSizeMultiplier);
}
largeHeaderBuffer = ArrayPool<byte>.Shared.Rent(_headersEncodingLargeBufferSize);
buffer = largeHeaderBuffer.AsSpan(0, _headersEncodingLargeBufferSize);
}
else
{
// In case of Done or MoreHeaders: write to output.
SplitHeaderAcrossFrames(streamId, buffer[..payloadLength], endOfHeaders: writeResult == HeaderWriteResult.Done, isFramePrepared: false);
}
}
if (largeHeaderBuffer != null)
{
ArrayPool<byte>.Shared.Return(largeHeaderBuffer);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in a finally?

Copy link
Contributor Author

@ladeak ladeak Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

Expand Down Expand Up @@ -1023,4 +1102,4 @@ private void EnqueueWaitingForMoreConnectionWindow(Http2OutputProducer producer)
_http2Connection.Abort(new ConnectionAbortedException("HTTP/2 connection exceeded the outgoing flow control maximum queue size."));
}
}
}
}
Loading
Loading