From 331725ab1651f189f622d85a267bbe4b7e5a5f9d Mon Sep 17 00:00:00 2001 From: Brennan Date: Sun, 7 Apr 2024 19:53:36 -0700 Subject: [PATCH 1/4] Fix Abort lock --- .../Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index f90f728a34fe..fead18298694 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -380,9 +380,12 @@ public void Complete() } _completed = true; - AbortConnectionFlowControl(); _outputWriter.Abort(); } + + // Ok to call after aborting the Pipe because we've already set _completed to true which means any writes from the abort call + // won't call into the Pipe. + AbortConnectionFlowControl(); } public Task ShutdownAsync() @@ -925,6 +928,9 @@ private void ConsumeConnectionWindow(long bytes) } } + /// + /// Do not call this method under the _writeLock + /// private void AbortConnectionFlowControl() { lock (_windowUpdateLock) From cebb459fc7927789c9dc3821892de8e3d48c5d4d Mon Sep 17 00:00:00 2001 From: Brennan Date: Mon, 8 Apr 2024 17:12:38 -0700 Subject: [PATCH 2/4] better --- .../src/Internal/Http2/Http2FrameWriter.cs | 27 +++++++++++++--- .../src/Internal/Http2/Http2OutputProducer.cs | 31 ++++++++++++------- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index fead18298694..a7e329fc59d5 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -370,17 +370,31 @@ public void UpdateMaxFrameSize(uint maxFrameSize) } } + /// + /// Call while in the _writeLock. + /// + /// true if already completed. + private bool CompleteUnsynchronized() + { + if (_completed) + { + return true; + } + + _completed = true; + _outputWriter.Abort(); + + return false; + } + public void Complete() { lock (_writeLock) { - if (_completed) + if (CompleteUnsynchronized()) { return; } - - _completed = true; - _outputWriter.Abort(); } // Ok to call after aborting the Pipe because we've already set _completed to true which means any writes from the abort call @@ -407,7 +421,10 @@ public void Abort(ConnectionAbortedException error) _aborted = true; _connectionContext.Abort(error); - Complete(); + if (CompleteUnsynchronized()) + { + return; + } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 8f13acbc2763..53d2299d2a33 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -590,6 +590,7 @@ public void Reset() internal void OnRequestProcessingEnded() { + var shouldCompleteStream = false; lock (_dataWriterLock) { if (_requestProcessingComplete) @@ -600,15 +601,22 @@ internal void OnRequestProcessingEnded() _requestProcessingComplete = true; - if (_completedResponse) - { - Stream.CompleteStream(errored: false); - } + shouldCompleteStream = _completedResponse; + } + + // Complete outside of lock, anything this method does that needs a lock will acquire a lock itself. + if (shouldCompleteStream) + { + Stream.CompleteStream(errored: false); } + } internal ValueTask CompleteResponseAsync() { + var shouldCompleteStream = false; + ValueTask task = default; + lock (_dataWriterLock) { if (_completedResponse) @@ -619,8 +627,6 @@ internal ValueTask CompleteResponseAsync() _completedResponse = true; - ValueTask task = default; - if (_resetErrorCode is { } error) { // If we have an error code to write, write it now that we're done with the response. @@ -628,13 +634,16 @@ internal ValueTask CompleteResponseAsync() task = _frameWriter.WriteRstStreamAsync(StreamId, error); } - if (_requestProcessingComplete) - { - Stream.CompleteStream(errored: false); - } + shouldCompleteStream = _requestProcessingComplete; + } - return task; + // Complete outside of lock, anything this method does that needs a lock will acquire a lock itself. + if (shouldCompleteStream) + { + Stream.CompleteStream(errored: false); } + + return task; } internal Memory GetFakeMemory(int minSize) From b076b7d8c3e18e5777cbbcd6ec8681fc7d3fb0ee Mon Sep 17 00:00:00 2001 From: Brennan Date: Wed, 10 Apr 2024 11:34:22 -0700 Subject: [PATCH 3/4] some updates --- .../Core/src/Internal/Http2/Http2FrameWriter.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index a7e329fc59d5..0c8c3ccea149 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -371,7 +371,7 @@ public void UpdateMaxFrameSize(uint maxFrameSize) } /// - /// Call while in the _writeLock. + /// Call while in the . /// /// true if already completed. private bool CompleteUnsynchronized() @@ -397,8 +397,8 @@ public void Complete() } } - // Ok to call after aborting the Pipe because we've already set _completed to true which means any writes from the abort call - // won't call into the Pipe. + // Call outside of _writeLock as this can call Http2OutputProducer.Stop which can acquire Http2OutputProducer._dataWriterLock + // which is not the desired lock order AbortConnectionFlowControl(); } @@ -426,6 +426,10 @@ public void Abort(ConnectionAbortedException error) return; } } + + // Call outside of _writeLock as this can call Http2OutputProducer.Stop which can acquire Http2OutputProducer._dataWriterLock + // which is not the desired lock order + AbortConnectionFlowControl(); } private ValueTask FlushEndOfStreamHeadersAsync(Http2Stream stream) @@ -946,7 +950,9 @@ private void ConsumeConnectionWindow(long bytes) } /// - /// Do not call this method under the _writeLock + /// Do not call this method under the _writeLock. + /// This method can call Http2OutputProducer.Stop which can acquire Http2OutputProducer._dataWriterLock + /// which is not the desired lock order /// private void AbortConnectionFlowControl() { From 146cf0a4f150d42bd88dc6b6271f5d2512256106 Mon Sep 17 00:00:00 2001 From: Brennan Date: Thu, 11 Apr 2024 09:27:28 -0700 Subject: [PATCH 4/4] comment + rename --- .../Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs | 6 +++--- .../Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index 0c8c3ccea149..3006288f4b8c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -502,7 +502,7 @@ private void WriteResponseHeadersUnsynchronized(int streamId, int statusCode, Ht _outgoingFrame.PrepareHeaders(headerFrameFlags, streamId); var buffer = _headerEncodingBuffer.AsSpan(); var done = HPackHeaderWriter.BeginEncodeHeaders(statusCode, _hpackEncoder, _headersEnumerator, buffer, out var payloadLength); - FinishWritingHeaders(streamId, payloadLength, done); + FinishWritingHeadersUnsynchronized(streamId, payloadLength, done); } // 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. @@ -543,7 +543,7 @@ private ValueTask WriteDataAndTrailersAsync(Http2Stream stream, in _outgoingFrame.PrepareHeaders(Http2HeadersFrameFlags.END_STREAM, streamId); var buffer = _headerEncodingBuffer.AsSpan(); var done = HPackHeaderWriter.BeginEncodeHeaders(_hpackEncoder, _headersEnumerator, buffer, out var payloadLength); - FinishWritingHeaders(streamId, payloadLength, done); + FinishWritingHeadersUnsynchronized(streamId, payloadLength, done); } // 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. @@ -557,7 +557,7 @@ private ValueTask WriteDataAndTrailersAsync(Http2Stream stream, in } } - private void FinishWritingHeaders(int streamId, int payloadLength, bool done) + private void FinishWritingHeadersUnsynchronized(int streamId, int payloadLength, bool done) { var buffer = _headerEncodingBuffer.AsSpan(); _outgoingFrame.PayloadLength = payloadLength; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 53d2299d2a33..f65890adf7cb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -605,6 +605,8 @@ internal void OnRequestProcessingEnded() } // Complete outside of lock, anything this method does that needs a lock will acquire a lock itself. + // Additionally, this method should only be called once per Reset so calling outside of the lock is fine from the perspective + // of multiple threads calling OnRequestProcessingEnded. if (shouldCompleteStream) { Stream.CompleteStream(errored: false); @@ -638,6 +640,8 @@ internal ValueTask CompleteResponseAsync() } // Complete outside of lock, anything this method does that needs a lock will acquire a lock itself. + // CompleteResponseAsync also should never be called in parallel so calling this outside of the lock doesn't + // cause any weirdness with parallel threads calling this method and no longer waiting on the stream completion call. if (shouldCompleteStream) { Stream.CompleteStream(errored: false);