-
Notifications
You must be signed in to change notification settings - Fork 523
Implement MaxFrameSize and HeaderTableSize for HTTP/2 #2838
Conversation
9fcf9c1
to
1de5817
Compare
@@ -89,6 +89,8 @@ public Http2Connection(Http2ConnectionContext context) | |||
_frameWriter = new Http2FrameWriter(context.Transport.Output, context.ConnectionContext, _outputFlowControl, this, context.ConnectionId, context.ServiceContext.Log); | |||
_hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); | |||
_serverSettings.MaxConcurrentStreams = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxStreamsPerConnection; | |||
_serverSettings.MaxFrameSize = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize; | |||
_incomingFrame = new Http2Frame(_serverSettings.MaxFrameSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allocates a 16 MB buffer heap allocated buffer per connection... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea... we haven't really optimized any of this and have been allocating all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 KB by default, but we should definitely used pooled memory for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this now? I think there are tons of allocations everywhere and we should do it systematically at some point. But I suppose we could start piecemeal at a time.
@@ -21,7 +21,7 @@ public Http2ContinuationFrameFlags ContinuationFlags | |||
|
|||
public void PrepareContinuation(Http2ContinuationFrameFlags flags, int streamId) | |||
{ | |||
PayloadLength = MinAllowedMaxFrameSize - HeaderLength; | |||
PayloadLength = (int)_maxFrameSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this since it always has to be set afterwards.
@@ -89,6 +89,8 @@ public Http2Connection(Http2ConnectionContext context) | |||
_frameWriter = new Http2FrameWriter(context.Transport.Output, context.ConnectionContext, _outputFlowControl, this, context.ConnectionId, context.ServiceContext.Log); | |||
_hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); | |||
_serverSettings.MaxConcurrentStreams = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxStreamsPerConnection; | |||
_serverSettings.MaxFrameSize = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize; | |||
_incomingFrame = new Http2Frame(_serverSettings.MaxFrameSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 KB by default, but we should definitely used pooled memory for this.
@@ -42,7 +42,7 @@ public void PrepareData(int streamId, byte? padLength = null) | |||
{ | |||
var padded = padLength != null; | |||
|
|||
PayloadLength = MinAllowedMaxFrameSize; | |||
PayloadLength = (int)_maxFrameSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line
@@ -141,7 +141,7 @@ public Http2TestBase() | |||
|
|||
_echoApplication = async context => | |||
{ | |||
var buffer = new byte[Http2Frame.MinAllowedMaxFrameSize]; | |||
var buffer = new byte[Http2Limits.MaxAllowedMaxFrameSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MinAllowedMaxFrameSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a test that I added that needs to echo a data frame that's bigger than the MinAllowedMaxFrameSize
and I wanted to reuse the _echoApplication. I could write a separate request delegate so that each test doesn't allocate 16 MB though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I forgot I restructured the test in question so this limit increase is no longer needed.
|
||
_clientSettings.Update(_incomingFrame.GetSettings()); | ||
|
||
if (_clientSettings.MaxFrameSize != previousMaxFrameSize) | ||
{ | ||
_frameWriter.UpdateMaxFrameSize(_clientSettings.MaxFrameSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: do all the business logic either before or after acking. Don't ack in the middle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the ack to the start since there's a comment saying it should be done before updating the window. But I'm curious since I don't see why the current implementation has any downsides?
@@ -64,7 +64,7 @@ public byte HeadersPriorityWeight | |||
|
|||
public void PrepareHeaders(Http2HeadersFrameFlags flags, int streamId) | |||
{ | |||
PayloadLength = MinAllowedMaxFrameSize - HeaderLength; | |||
PayloadLength = (int)_maxFrameSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line
@@ -89,6 +89,8 @@ public Http2Connection(Http2ConnectionContext context) | |||
_frameWriter = new Http2FrameWriter(context.Transport.Output, context.ConnectionContext, _outputFlowControl, this, context.ConnectionId, context.ServiceContext.Log); | |||
_hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); | |||
_serverSettings.MaxConcurrentStreams = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxStreamsPerConnection; | |||
_serverSettings.MaxFrameSize = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of these conversions between int and uint everywhere. Currently the values on Limits.Http2 are all ints and the values on Http2PeerSettings are uints so we have to convert between the two. We should just pick one and stick to it.
Also addressing #2816 in the same PR since it's an even smaller change. |
_serverSettings.MaxConcurrentStreams = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxStreamsPerConnection; | ||
_serverSettings.MaxFrameSize = (uint)context.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize; | ||
_serverSettings.HeaderTableSize = (uint)context.ServiceContext.ServerOptions.Limits.Http2.HeaderTableSize; | ||
_hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no corresponding logic required to handle the HeaderTableSize of the _clientSettings since we are not currently doing header compression in our HPackEncoder. Our HPackEncoder currently does a direct literal encoding without using a compression table or huffman encoding; I should file an issue for that.
89a37da
to
1a02735
Compare
🆙📅 |
@davidfowl https://ci3.dot.net/job/aspnet_KestrelHttpServer/job/release_2.2/job/linux-Configuration_Debug_prtest/159/ looks like a regression possibly caused by your recent change.
Notice that the missing trace, "Connection id "0HLG9A5H9REP4" stopped.", was indeed logged, but not before the mock verification which occurs after the WebHost is disposed. There also aren't any logs indication ungraceful shutdown. |
await StartStreamAsync(1, _browserRequestHeaders, endStream: true); | ||
|
||
await ExpectAsync(Http2FrameType.HEADERS, | ||
withLength: 16390, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be calculated from MinAllowedMaxFrameSize?
f45cfe8
to
64127e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor cleanup
private int _maxFrameSize = MinAllowedMaxFrameSize; | ||
|
||
// These are limits defined by the RFC https://tools.ietf.org/html/rfc7540#section-4.2 | ||
public const int MaxAllowedHeaderTableSize = 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these constants public?
They should also be listed above members.
/// <summary> | ||
/// Limits the size of the header compression table, in octets, the HPACK decoder on the server can use. | ||
/// <para> | ||
/// Defaults to 4096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give the valid range.
// Ack before we update the windows, they could send data immediately. | ||
await _frameWriter.WriteSettingsAckAsync(); | ||
|
||
if (_clientSettings.MaxFrameSize != previousMaxFrameSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do this before writing the settings ack to avoid the async await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I originally had, but @halter73 wanted it moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you just store the task in a local and return it like before?
_connectionContext.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize = length; | ||
_connection = new Http2Connection(_connectionContext); | ||
|
||
await InitializeConnectionAsync(_echoApplication, expectedSettingsLegnth: 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legnth -> Length. And I'd rather it be Count: 3
withLength: 37, | ||
withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, | ||
withStreamId: 1); | ||
// The client's settings is still defaulted to Http2PeerSettings.MinAllowedMaxFrameSize so the echo response will come back in two separate frames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is out of date
public Http2Frame(uint maxFrameSize) | ||
{ | ||
_maxFrameSize = maxFrameSize; | ||
_data = new byte[HeaderLength + _maxFrameSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be allocating the max size. #2858
@@ -11,6 +11,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core | |||
public class Http2Limits | |||
{ | |||
private int _maxStreamsPerConnection = 100; | |||
private int _headerTableSize = MaxAllowedHeaderTableSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http2PeerSettings.DefaultHeaderTableSize
Addresses #2817