Don't pause and resume read timing on upgrade requests (#1904).

Author: Cesar Blum Silveira

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

@@ -8,6 +8,7 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Internal.System.IO.Pipelines;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
 {
@@ -92,14 +93,14 @@ private async Task PumpAsync()
                             {
                                 // Backpressure, stop controlling incoming data rate until data is read.
                                 backpressure = true;
-                                _context.TimeoutControl.PauseTimingReads();
+                                TryPauseTimingReads();
                             }
 
                             await writeAwaitable;
 
                             if (backpressure)
                             {
-                                _context.TimeoutControl.ResumeTimingReads();
+                                TryResumeTimingReads();
                             }
 
                             if (done)
@@ -273,6 +274,22 @@ private void TryStartTimingReads()
             }
         }
 
+        private void TryPauseTimingReads()
+        {
+            if (!RequestUpgrade)
+            {
+                _context.TimeoutControl.PauseTimingReads();
+            }
+        }
+
+        private void TryResumeTimingReads()
+        {
+            if (!RequestUpgrade)
+            {
+                _context.TimeoutControl.ResumeTimingReads();
+            }
+        }
+
         private void TryStopTimingReads()
         {
             if (!RequestUpgrade)

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

@@ -648,6 +648,29 @@ public async Task LogsWhenStopsReadingRequestBody()
             }
         }
 
+        [Fact]
+        public async Task PausesAndResumesRequestBodyTimeoutOnBackpressure()
+        {
+            using (var input = new TestInput())
+            {
+                var mockTimeoutControl = new Mock<ITimeoutControl>();
+                input.FrameContext.TimeoutControl = mockTimeoutControl.Object;
+
+                var body = MessageBody.For(HttpVersion.Http11, new FrameRequestHeaders { HeaderContentLength = "12" }, input.Frame);
+
+                // Add some input and read it to start PumpAsync
+                input.Add("hello,");
+                Assert.Equal(6, await body.ReadAsync(new ArraySegment<byte>(new byte[6])));
+
+                input.Add(" world");
+                Assert.Equal(6, await body.ReadAsync(new ArraySegment<byte>(new byte[6])));
+
+                // Due to the limits set on Frame.RequestBodyPipe, backpressure should be triggered on every write to that pipe.
+                mockTimeoutControl.Verify(timeoutControl => timeoutControl.PauseTimingReads(), Times.Exactly(2));
+                mockTimeoutControl.Verify(timeoutControl => timeoutControl.ResumeTimingReads(), Times.Exactly(2));
+            }
+        }
+
         [Fact]
         public async Task OnlyEnforcesRequestBodyTimeoutAfterSending100Continue()
         {
@@ -701,6 +724,12 @@ public async Task DoesNotEnforceRequestBodyTimeoutOnUpgradeRequests()
 
                 mockTimeoutControl.Verify(timeoutControl => timeoutControl.StartTimingReads(), Times.Never);
                 mockTimeoutControl.Verify(timeoutControl => timeoutControl.StopTimingReads(), Times.Never);
+
+                // Due to the limits set on Frame.RequestBodyPipe, backpressure should be triggered on every
+                // write to that pipe. Verify that read timing pause and resume are not called on upgrade
+                // requests.
+                mockTimeoutControl.Verify(timeoutControl => timeoutControl.PauseTimingReads(), Times.Never);
+                mockTimeoutControl.Verify(timeoutControl => timeoutControl.ResumeTimingReads(), Times.Never);
             }
         }
 

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

@@ -17,7 +17,9 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.AspNetCore.Testing.xunit;
@@ -1416,6 +1418,68 @@ await connection.ReceiveForcedEnd(
             }
         }
 
+        [Fact]
+        public async Task DoesNotEnforceRequestBodyMinimumDataRateOnUpgradedRequest()
+        {
+            var appEvent = new ManualResetEventSlim();
+            var delayEvent = new ManualResetEventSlim();
+            var serviceContext = new TestServiceContext
+            {
+                SystemClock = new SystemClock()
+            };
+
+            using (var server = new TestServer(async context =>
+            {
+                context.Features.Get<IHttpRequestBodyMinimumDataRateFeature>().MinimumDataRate = new MinimumDataRate(rate: double.MaxValue, gracePeriod: TimeSpan.Zero);
+
+                using (var stream = await context.Features.Get<IHttpUpgradeFeature>().UpgradeAsync())
+                {
+                    appEvent.Set();
+
+                    // Read once to go through one set of TryPauseTimingReads()/TryResumeTimingReads() calls
+                    await stream.ReadAsync(new byte[1], 0, 1);
+
+                    delayEvent.Wait();
+
+                    // Read again to check that the connection is still alive
+                    await stream.ReadAsync(new byte[1], 0, 1);
+
+                    // Send a response to distinguish from the timeout case where the 101 is still received, but without any content
+                    var response = Encoding.ASCII.GetBytes("hello");
+                    await stream.WriteAsync(response, 0, response.Length);
+                }
+            }, serviceContext))
+            {
+                using (var connection = server.CreateConnection())
+                {
+                    await connection.Send(
+                        "GET / HTTP/1.1",
+                        "Host:",
+                        "Connection: upgrade",
+                        "",
+                        "a");
+
+                    Assert.True(appEvent.Wait(TimeSpan.FromSeconds(10)));
+
+                    await Task.Delay(TimeSpan.FromSeconds(5));
+
+                    delayEvent.Set();
+
+                    await connection.Send("b");
+
+                    await connection.Receive(
+                        "HTTP/1.1 101 Switching Protocols",
+                        "Connection: Upgrade",
+                        "");
+                    await connection.ReceiveStartsWith(
+                        $"Date: ");
+                    await connection.ReceiveForcedEnd(
+                        "",
+                        "hello");
+                }
+            }
+        }
+
         private async Task TestRemoteIPAddress(string registerAddress, string requestAddress, string expectAddress)
         {
             var builder = new WebHostBuilder()