Implement HTTP/2 input flow control#2740
Conversation
| { | ||
| _flow = new Http2FlowControl(initialWindowSize); | ||
| _initialWindowSize = (int)initialWindowSize; | ||
| _minWindowSizeIncrement = _initialWindowSize / 2; |
There was a problem hiding this comment.
32kb? That seems high when TLS frames are limited to 16kb. 8-16kb?
I know you're not exposing settings yet, but do you want to plum this through as a parameter now so it will be easier to configure later?
|
|
||
| using System.Diagnostics; | ||
|
|
||
| namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 |
There was a problem hiding this comment.
Http2.FlowControl to match the directory?
|
|
||
| namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 | ||
| { | ||
| public class Http2StreamInputFlowControl |
There was a problem hiding this comment.
StreamInputFlowControl? No need to include Http2 in all the type names when it's in the namespace there's nothing to disambiguate against (e.g. Http2Connection and Http2Stream make sense, but most of these others don't need it).
| if (streamWindowUpdateSize > 0) | ||
| { | ||
| // Writing with the FrameWriter should only fail if given a canceled token, so just fire and forget. | ||
| _ = _frameWriter.WriteWindowUpdateAsync(_streamId, streamWindowUpdateSize); |
There was a problem hiding this comment.
Any way to batch stream window updates and connection window updates into the same write operation? A connection window update will primarily be triggered by a stream update right? (With the exception of a stream abort?)
There was a problem hiding this comment.
Stream and connection window updates will only happen at the same time when there's only one stream. With multiple streams, the windows get out of sync. Once we introduce a different window size for the connection vs the streams, the windows won't even be in sync in the one stream scenario.
| _context.OnDataRead(bytesRead); | ||
| } | ||
|
|
||
| // REVIEW: Can an app partially consume the request body in a non-aborted stream? Write a test. |
| RequestBodyPipe.Writer.Complete(new IOException(CoreStrings.Http2StreamAborted, abortReason)); | ||
|
|
||
| // Stop counting any buffered bytes for this stream towards the connection input flow-control window. | ||
| _inputFlowControl.Abort(); |
There was a problem hiding this comment.
This needs to update the connection flow control.
- Disable late memory pool block tracking in some tests - add minWindowSizeIncrement param
|
⬆️📅 |
|
@dotnet-bot test OSX 10.12 Release Build |
| public bool EndStreamReceived { get; private set; } | ||
|
|
||
| protected IHttp2StreamLifetimeHandler StreamLifetimeHandler => _context.StreamLifetimeHandler; | ||
| public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; |
There was a problem hiding this comment.
I see what you meant by wanting to use HasFlag
There was a problem hiding this comment.
What's wrong with HasFlag? Is an intrinsic now dotnet/coreclr#13748
There was a problem hiding this comment.
That change is what got us talking about HasFlag. We don't want to rely on CoreCLR-only intrinsics yet. In this case, you could argue it's OK since the desktop framework doesn't support ALPN, but we'd rather switch to using HasFlag all at once.
There was a problem hiding this comment.
The method exists in desktop though? Its just slower?
There was a problem hiding this comment.
Yep. The desktop version of HasFlag does reflection each time IIRC.
| var lastCompletionState = _completionState; | ||
| _completionState |= completionState; | ||
|
|
||
| if (ShoulStopTrackingStream(_completionState) && !ShoulStopTrackingStream(lastCompletionState)) |
There was a problem hiding this comment.
Missing d
ShoulStopTrackingStream
ShouldStopTrackingStream
3 participants
#2710