diff --git a/src/Kestrel.Core/Internal/ConnectionDispatcher.cs b/src/Kestrel.Core/Internal/ConnectionDispatcher.cs index 90aedba443..79614ec51b 100644 --- a/src/Kestrel.Core/Internal/ConnectionDispatcher.cs +++ b/src/Kestrel.Core/Internal/ConnectionDispatcher.cs @@ -59,6 +59,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal { Log.LogCritical(0, ex, $"{nameof(ConnectionDispatcher)}.{nameof(Execute)}() {connectionContext.ConnectionId}"); } + finally + { + // Complete the transport PipeReader and PipeWriter after calling into application code + connectionContext.Transport.Input.Complete(); + connectionContext.Transport.Output.Complete(); + } } } diff --git a/test/Kestrel.Core.Tests/ConnectionDispatcherTests.cs b/test/Kestrel.Core.Tests/ConnectionDispatcherTests.cs index c9acd1c658..cbf4c4b88b 100644 --- a/test/Kestrel.Core.Tests/ConnectionDispatcherTests.cs +++ b/test/Kestrel.Core.Tests/ConnectionDispatcherTests.cs @@ -1,7 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; +using System.IO.Pipelines; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; @@ -42,5 +44,27 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Verify the scope was disposed after request processing completed Assert.True(((TestKestrelTrace)serviceContext.Log).Logger.Scopes.IsEmpty); } + + [Fact] + public async Task OnConnectionCompletesTransportPipesAfterReturning() + { + var serviceContext = new TestServiceContext(); + var tcs = new TaskCompletionSource(); + var dispatcher = new ConnectionDispatcher(serviceContext, _ => Task.CompletedTask); + + var mockConnection = new Mock(); + var mockPipeReader = new Mock(); + var mockPipeWriter = new Mock(); + var mockPipe = new Mock(); + mockPipe.Setup(m => m.Input).Returns(mockPipeReader.Object); + mockPipe.Setup(m => m.Output).Returns(mockPipeWriter.Object); + mockConnection.Setup(m => m.Transport).Returns(mockPipe.Object); + var connection = mockConnection.Object; + + await dispatcher.OnConnection(connection); + + mockPipeWriter.Verify(m => m.Complete(It.IsAny()), Times.Once()); + mockPipeReader.Verify(m => m.Complete(It.IsAny()), Times.Once()); + } } } diff --git a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs index 997285ef4e..56ae2c8095 100644 --- a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs +++ b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs @@ -131,14 +131,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests public Http2ConnectionTests() { - var inlineSchedulingPipeOptions = new PipeOptions( + // Always dispatch test code back to the ThreadPool. This prevents deadlocks caused by continuing + // Http2Connection.ProcessRequestsAsync() loop with writer locks acquired. Run product code inline to make + // it easier to verify request frames are processed correctly immediately after sending the them. + var inputPipeOptions = new PipeOptions( pool: _memoryPool, readerScheduler: PipeScheduler.Inline, + writerScheduler: PipeScheduler.ThreadPool, + useSynchronizationContext: false + ); + var outputPipeOptions = new PipeOptions( + pool: _memoryPool, + readerScheduler: PipeScheduler.ThreadPool, writerScheduler: PipeScheduler.Inline, useSynchronizationContext: false ); - _pair = DuplexPipe.CreateConnectionPair(inlineSchedulingPipeOptions, inlineSchedulingPipeOptions); + _pair = DuplexPipe.CreateConnectionPair(inputPipeOptions, outputPipeOptions); _noopApplication = context => Task.CompletedTask; @@ -488,12 +497,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withFlags: (byte)Http2DataFrameFlags.END_STREAM, withStreamId: 3); - // Ensure that Http2FrameWriter._writeLock isn't acquired when completing the frame processing loop. - // Otherwise there's a deadlock where Http2OutputProducer.Abort() being called from the frame processing - // loop blocks waiting Http2OutputProducer.Dispose() being called from the stream processing loop to - // acquire the _writeLock. - await ThreadPoolAwaitable.Instance; - await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); Assert.Equal(stream1DataFrame1.DataPayload, _helloBytes); @@ -565,12 +568,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withFlags: (byte)Http2DataFrameFlags.END_STREAM, withStreamId: 1); - // Ensure that Http2FrameWriter._writeLock isn't acquired when completing the frame processing loop. - // Otherwise there's a deadlock where Http2OutputProducer.Abort() being called from the frame processing - // loop blocks waiting Http2OutputProducer.Dispose() being called from the stream processing loop to - // acquire the _writeLock. - await ThreadPoolAwaitable.Instance; - await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); } @@ -868,12 +865,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withFlags: (byte)Http2DataFrameFlags.END_STREAM, withStreamId: 1); - // Ensure that Http2FrameWriter._writeLock isn't acquired when completing the frame processing loop. - // Otherwise there's a deadlock where Http2OutputProducer.Abort() being called from the frame processing - // loop blocks waiting Http2OutputProducer.Dispose() being called from the stream processing loop to - // acquire the _writeLock. - await ThreadPoolAwaitable.Instance; - await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); await WaitForAllStreamsAsync(); }