From 7afd279a6dd6c36712d74e8bfde86cbddc33a422 Mon Sep 17 00:00:00 2001 From: Cesar Blum Silveira Date: Mon, 19 Jun 2017 16:37:47 -0700 Subject: [PATCH] Don't pause and resume read timing on upgrade requests (#1904). --- .../Internal/Http/MessageBody.cs | 21 +++++- .../MessageBodyTests.cs | 29 +++++++++ .../RequestTests.cs | 64 +++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs index f964f55001..0e07e1d0a3 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs @@ -8,6 +8,7 @@ using System.Threading.Tasks; 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 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { // 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 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } } + private void TryPauseTimingReads() + { + if (!RequestUpgrade) + { + _context.TimeoutControl.PauseTimingReads(); + } + } + + private void TryResumeTimingReads() + { + if (!RequestUpgrade) + { + _context.TimeoutControl.ResumeTimingReads(); + } + } + private void TryStopTimingReads() { if (!RequestUpgrade) diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs index b884baa874..c8dfaccfe6 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs @@ -648,6 +648,29 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests } } + [Fact] + public async Task PausesAndResumesRequestBodyTimeoutOnBackpressure() + { + using (var input = new TestInput()) + { + var mockTimeoutControl = new Mock(); + 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(new byte[6]))); + + input.Add(" world"); + Assert.Equal(6, await body.ReadAsync(new ArraySegment(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 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests 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); } } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs index 999e4f78a3..da34b13bd6 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs @@ -17,7 +17,9 @@ using Microsoft.AspNetCore.Hosting; 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 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests } } + [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().MinimumDataRate = new MinimumDataRate(rate: double.MaxValue, gracePeriod: TimeSpan.Zero); + + using (var stream = await context.Features.Get().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()