Always complete RequestBodyPipe.Reader (#1893)

* Disable test that leaks blocks in debug builds
* Move some test helpers

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameOfT.cs

@@ -163,13 +163,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                                         TimeoutControl.SetTimeout(Constants.RequestBodyDrainTimeout.Ticks, TimeoutAction.SendTimeoutResponse);
                                         await messageBody.ConsumeAsync();
                                         TimeoutControl.CancelTimeout();
-
-                                        // At this point both the request body pipe reader and writer should be completed.
-                                        RequestBodyPipe.Reset();
                                     }
                                     else
                                     {
-                                        RequestBodyPipe.Reader.Complete();
                                         messageBody.Cancel();
                                         Input.CancelPendingRead();
                                     }
@@ -201,6 +197,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                             // StopStreams should be called before the end of the "if (!_requestProcessingStopping)" block
                             // to ensure InitializeStreams has been called.
                             StopStreams();
+
+                            if (HasStartedConsumingRequestBody)
+                            {
+                                RequestBodyPipe.Reader.Complete();
+
+                                // At this point both the request body pipe reader and writer should be completed.
+                                RequestBodyPipe.Reset();
+                            }
                         }
                     }
 

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

@@ -204,19 +204,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
         {
             TryInit();
 
-            try
+            ReadResult result;
+            do
             {
-                ReadResult result;
-                do
-                {
-                    result = await _context.RequestBodyPipe.Reader.ReadAsync();
-                    _context.RequestBodyPipe.Reader.Advance(result.Buffer.End);
-                } while (!result.IsCompleted);
-            }
-            finally
-            {
-                _context.RequestBodyPipe.Reader.Complete();
-            }
+                result = await _context.RequestBodyPipe.Reader.ReadAsync();
+                _context.RequestBodyPipe.Reader.Advance(result.Buffer.End);
+            } while (!result.IsCompleted);
         }
 
         protected void Copy(ReadableBuffer readableBuffer, WritableBuffer writableBuffer)

test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs

@@ -349,7 +349,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
         }
 
         [Fact]
-        public async Task ConsumeAsyncCompletesPipeReader()
+        public async Task ConsumeAsyncConsumesAllRemainingInput()
         {
             using (var input = new TestInput())
             {
@@ -359,7 +359,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
 
                 await body.ConsumeAsync();
 
-                await Assert.ThrowsAsync<InvalidOperationException>(async () => await body.ReadAsync(new ArraySegment<byte>(new byte[1])));
+                Assert.Equal(0, await body.ReadAsync(new ArraySegment<byte>(new byte[1])));
             }
         }
 

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/FrameConnectionManagerTests.cs

@@ -16,7 +16,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
 {
     public class FrameConnectionManagerTests
     {
-
+// This test causes MemoryPoolBlocks to be finalized which in turn causes an assert failure in debug builds.
+#if !DEBUG
         [ConditionalFact]
         [NoDebuggerCondition]
         public async Task CriticalErrorLoggedIfApplicationDoesntComplete()
@@ -65,6 +66,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
                 Assert.True(logWaitAttempts < 10);
             }
         }
+#endif
 
         private class NoDebuggerConditionAttribute : Attribute, ITestCondition
         {