diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index f90f728a34fe..3006288f4b8c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -370,19 +370,36 @@ public void UpdateMaxFrameSize(uint maxFrameSize) } } + /// + /// Call while in the . + /// + /// 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; - AbortConnectionFlowControl(); - _outputWriter.Abort(); } + + // Call outside of _writeLock as this can call Http2OutputProducer.Stop which can acquire Http2OutputProducer._dataWriterLock + // which is not the desired lock order + AbortConnectionFlowControl(); } public Task ShutdownAsync() @@ -404,8 +421,15 @@ public void Abort(ConnectionAbortedException error) _aborted = true; _connectionContext.Abort(error); - Complete(); + if (CompleteUnsynchronized()) + { + 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) @@ -478,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. @@ -519,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. @@ -533,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; @@ -925,6 +949,11 @@ private void ConsumeConnectionWindow(long bytes) } } + /// + /// 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() { lock (_windowUpdateLock) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 8f13acbc2763..f65890adf7cb 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,24 @@ 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. + // 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); } + } internal ValueTask CompleteResponseAsync() { + var shouldCompleteStream = false; + ValueTask task = default; + lock (_dataWriterLock) { if (_completedResponse) @@ -619,8 +629,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 +636,18 @@ 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. + // 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); } + + return task; } internal Memory GetFakeMemory(int minSize)