Skip to content

HTTP/2 output processing make over #40925

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.

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

Merged
merged 77 commits into from
Apr 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
e0d3bbf
WIP
davidfowl Mar 23, 2022
f478542
Make more things work
davidfowl Mar 23, 2022
d2a5eb5
No more crashes!
davidfowl Mar 23, 2022
6f07604
Added trailers
davidfowl Mar 23, 2022
e70cb39
Delete more code so it's obvious where changes are in the diff
davidfowl Mar 23, 2022
1d5ba97
Complete the channel in Complete
davidfowl Mar 23, 2022
c60d113
Move to flow control to the FrameWriter instead
davidfowl Mar 23, 2022
b14cf57
Use correct lock, add logs
davidfowl Mar 23, 2022
8a59b46
Abort flow control in a single place
davidfowl Mar 23, 2022
01905ee
Remove dead code
davidfowl Mar 23, 2022
30ef07d
Undo chanes
davidfowl Mar 23, 2022
3c3d4f8
Remove call
davidfowl Mar 23, 2022
086c627
Made a few tests pass
davidfowl Mar 24, 2022
c72680b
"Fixed" more tests
davidfowl Mar 24, 2022
6257f26
Put back some timeouts
davidfowl Mar 24, 2022
82fe5cd
Made more test fixes
davidfowl Mar 25, 2022
59b51e4
Fix how we stop streams when flow control is being aborted
davidfowl Mar 25, 2022
da54396
Always clean up the pipe writer
davidfowl Mar 25, 2022
8f841d4
Fixed more tests
davidfowl Mar 25, 2022
645479a
Make inline scheduling work better
davidfowl Mar 26, 2022
033c54d
Enqueue unflushed bytes before setting observation flag
davidfowl Mar 26, 2022
78a1ea9
Moar fixes
davidfowl Mar 26, 2022
4c88f2f
Cleaned up some tests
davidfowl Mar 27, 2022
f57ce67
Remove space
davidfowl Mar 27, 2022
4dfe135
More code cleanup
davidfowl Mar 27, 2022
4d521ad
Made the timeout tests work
davidfowl Mar 27, 2022
af84669
Tests pass!
davidfowl Mar 27, 2022
533b6a6
Move header writing to the processing loop
davidfowl Mar 27, 2022
1a3d0f0
Small clean up
davidfowl Mar 27, 2022
17c9cc6
Merged the locks for header writing when possible
davidfowl Mar 27, 2022
8593ef2
More optimizations
davidfowl Mar 28, 2022
6dbfb14
Remove a state machine from the write code path
davidfowl Mar 28, 2022
69a0384
Rename firstWrite to writeHeaders
davidfowl Mar 28, 2022
4b01192
Make the channel unboundeded
davidfowl Mar 29, 2022
42425d9
Shutdown the write queue after the connection loop ends
davidfowl Mar 29, 2022
eb78e17
Make James happy
davidfowl Mar 29, 2022
9d80ad4
Rename schedule again to reschedule
davidfowl Mar 29, 2022
2b31f99
Rename cancelled to use one l
davidfowl Mar 29, 2022
7bb3afb
Spelling
davidfowl Mar 29, 2022
0b94cb2
Move trailer writing into a single method
davidfowl Mar 29, 2022
2626567
Remove extra lines...
davidfowl Mar 29, 2022
5b93caa
Remove task chain allocation on shutdown
davidfowl Mar 31, 2022
66a6d7e
Removed the use of the output flow control objects from the source
davidfowl Mar 31, 2022
56481d6
Delete output flow control types
davidfowl Mar 31, 2022
2388dce
Make ConsumeWindow private
davidfowl Mar 31, 2022
9b3be39
Remove if from queue processing
davidfowl Apr 1, 2022
de5787e
Oops
davidfowl Apr 1, 2022
aa495ea
Fixed logs
davidfowl Apr 1, 2022
9f54e60
Fix merge
JamesNK Apr 4, 2022
cb0e7e1
Don't reschedule for nothing
halter73 Apr 9, 2022
9b42561
Fix typo
halter73 Apr 11, 2022
a1d56b8
Apply PR feedback
davidfowl Apr 12, 2022
610ec1d
Removed unneeded state
davidfowl Apr 12, 2022
7a92bee
More PR suggestions
davidfowl Apr 12, 2022
de855de
Remove the FlushHeaders bool and use the unobserved state
davidfowl Apr 12, 2022
c3a1289
Small nits
davidfowl Apr 12, 2022
97e728d
Small tweaks
davidfowl Apr 12, 2022
69f204d
More naming changs
davidfowl Apr 12, 2022
66a8d4e
More PR feedback
davidfowl Apr 12, 2022
41d8e2e
Removed usings...
davidfowl Apr 12, 2022
7acfdd4
Aborts and writes are no longer synchronized
davidfowl Apr 12, 2022
600cffb
Preserve existing behavior
davidfowl Apr 12, 2022
243643d
Removed unused state
davidfowl Apr 12, 2022
6af2c0d
Make the test await the process request loop
davidfowl Apr 12, 2022
98963b7
Removed extra flush headers state.
davidfowl Apr 12, 2022
812964f
PR feedback
davidfowl Apr 12, 2022
7cc588d
Improve the fairness of connection window updates
davidfowl Apr 13, 2022
c243966
Fixed test
davidfowl Apr 13, 2022
6650be8
Increased pause threshold to 4K
davidfowl Apr 14, 2022
893ebd7
Simplify the queue fairness
davidfowl Apr 14, 2022
e41c7b3
Fixed 2 remaining issues
davidfowl Apr 14, 2022
1c542e1
Added last window consumer to the queue when aborting
davidfowl Apr 14, 2022
618f7f2
Stop for RST frames as well.
davidfowl Apr 15, 2022
6ab3f4f
Small tweaks to state management
davidfowl Apr 15, 2022
d6e0a09
This is the last commit that fixes all of the bugs (I hope...)
davidfowl Apr 16, 2022
77ff682
Make things "simpler"
davidfowl Apr 16, 2022
02319f0
We're not boxing today
davidfowl Apr 16, 2022
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

This file was deleted.

This file was deleted.

This file was deleted.

67 changes: 15 additions & 52 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,10 @@ internal partial class Http2Connection : IHttp2StreamLifetimeHandler, IHttpStrea
private readonly HttpConnectionContext _context;
private readonly Http2FrameWriter _frameWriter;
private readonly Pipe _input;
private readonly Pipe _output;
private readonly Task _inputTask;
private readonly Task _outputTask;
private readonly int _minAllocBufferSize;
private readonly HPackDecoder _hpackDecoder;
private readonly InputFlowControl _inputFlowControl;
private readonly OutputFlowControl _outputFlowControl = new OutputFlowControl(new MultipleAwaitableProvider(), Http2PeerSettings.DefaultInitialWindowSize);

private readonly Http2PeerSettings _serverSettings = new Http2PeerSettings();
private readonly Http2PeerSettings _clientSettings = new Http2PeerSettings();
Expand Down Expand Up @@ -74,6 +71,7 @@ internal partial class Http2Connection : IHttp2StreamLifetimeHandler, IHttpStrea
internal readonly Http2KeepAlive? _keepAlive;
internal readonly Dictionary<int, Http2Stream> _streams = new Dictionary<int, Http2Stream>();
internal PooledStreamStack<Http2Stream> StreamPool;
internal Action<Http2Stream>? _onStreamCompleted;
Copy link
Member

@JamesNK JamesNK Apr 16, 2022

Choose a reason for hiding this comment

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

Is this just for testing?

I don't think it needs to be fixed now, but I'd like to see this reworked to utilize IHttp2StreamLifetimeHandler. Tests should support customizing the handler so it can be used to raise this event. I did this in the HTTP/3 tests and it works well.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a follow-up issue.


public Http2Connection(HttpConnectionContext context)
{
Expand All @@ -86,18 +84,6 @@ public Http2Connection(HttpConnectionContext context)
_context.InitialExecutionContext = ExecutionContext.Capture();

_input = new Pipe(GetInputPipeOptions());
_output = new Pipe(GetOutputPipeOptions());

_frameWriter = new Http2FrameWriter(
_output.Writer,
context.ConnectionContext,
this,
_outputFlowControl,
context.TimeoutControl,
httpLimits.MinResponseDataRate,
context.ConnectionId,
context.MemoryPool,
context.ServiceContext);

_minAllocBufferSize = context.MemoryPool.GetMinimumAllocSize();

Expand Down Expand Up @@ -126,7 +112,17 @@ public Http2Connection(HttpConnectionContext context)
_scheduleInline = context.ServiceContext.Scheduler == PipeScheduler.Inline;

_inputTask = CopyPipeAsync(_context.Transport.Input, _input.Writer);
_outputTask = CopyPipeAsync(_output.Reader, _context.Transport.Output);

_frameWriter = new Http2FrameWriter(
context.Transport.Output,
context.ConnectionContext,
this,
(int)Math.Min(MaxTrackedStreams, int.MaxValue),
context.TimeoutControl,
httpLimits.MinResponseDataRate,
context.ConnectionId,
context.MemoryPool,
context.ServiceContext);
}

public string ConnectionId => _context.ConnectionId;
Expand Down Expand Up @@ -378,10 +374,9 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
finally
{
Input.Complete();
_output.Writer.Complete();
_context.Transport.Input.CancelPendingRead();
await _inputTask;
await _outputTask;
await _frameWriter.ShutdownAsync();
}
}
}
Expand Down Expand Up @@ -762,8 +757,7 @@ private Http2StreamContext CreateHttp2StreamContext()
_clientSettings,
_serverSettings,
_frameWriter,
_inputFlowControl,
_outputFlowControl);
_inputFlowControl);
streamContext.TimeoutControl = _context.TimeoutControl;
streamContext.InitialExecutionContext = _context.InitialExecutionContext;

Expand Down Expand Up @@ -1236,6 +1230,7 @@ void IHttp2StreamLifetimeHandler.OnStreamCompleted(Http2Stream stream)
{
_completedStreams.Enqueue(stream);
_streamCompletionAwaitable.Complete();
_onStreamCompleted?.Invoke(stream);
}

private void UpdateCompletedStreams()
Expand Down Expand Up @@ -1662,38 +1657,6 @@ public void DecrementActiveClientStreamCount()
minimumSegmentSize: _context.MemoryPool.GetMinimumSegmentSize(),
useSynchronizationContext: false);

private PipeOptions GetOutputPipeOptions()
{
// Never write inline because we do not want to hold Http2FramerWriter._writeLock for potentially expensive TLS
// write operations. This essentially doubles the MaxResponseBufferSize for HTTP/2 connections compared to
// HTTP/1.x. This seems reasonable given HTTP/2's support for many concurrent streams per connection. We don't
// want every write to return an incomplete ValueTask now that we're dispatching TLS write operations which
// would likely happen with a pauseWriterThreshold of 1, but we still need to respect connection back pressure.
var pauseWriterThreshold = _context.ServiceContext.ServerOptions.Limits.MaxResponseBufferSize switch
{
// null means that we have no back pressure
null => 0,
// 0 = no buffering so we need to configure the pipe so the writer waits on the reader directly
0 => 1,
long limit => limit,
};

var resumeWriterThreshold = pauseWriterThreshold switch
{
// The resumeWriterThreshold must be at least 1 to ever resume after pausing.
1 => 1,
long limit => limit / 2,
};

return new PipeOptions(pool: _context.MemoryPool,
readerScheduler: _context.ServiceContext.Scheduler,
writerScheduler: PipeScheduler.Inline,
pauseWriterThreshold: pauseWriterThreshold,
resumeWriterThreshold: resumeWriterThreshold,
minimumSegmentSize: _context.MemoryPool.GetMinimumSegmentSize(),
useSynchronizationContext: false);
}

private async Task CopyPipeAsync(PipeReader reader, PipeWriter writer)
{
Exception? error = null;
Expand Down
Loading