From 83bf2375b39d2f8f28c1314d1ae0a64e22f025e8 Mon Sep 17 00:00:00 2001 From: Justin Wyer Date: Mon, 26 Feb 2018 03:10:04 +0200 Subject: [PATCH] #2035 Do not await OnCompleted handlers before sending the Response (#2324) --- .../Internal/Http/HttpProtocol.cs | 18 ++- test/Kestrel.FunctionalTests/ResponseTests.cs | 106 +++++++++++++++++- 2 files changed, 109 insertions(+), 15 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index 274376c31b..7839a33cbd 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -554,11 +554,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http PauseStreams(); - if (_onCompleted != null) - { - await FireOnCompleted(); - } - if (badRequestException == null) { // If _requestAbort is set, the connection has already been closed. @@ -601,6 +596,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } } + if (_onCompleted != null) + { + await FireOnCompleted(); + } + if (badRequestException != null) { // Handle BadHttpRequestException thrown during app execution or remaining message body consumption. @@ -739,10 +739,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { return Task.CompletedTask; } - else - { - return FireOnCompletedAwaited(onCompleted); - } + + return FireOnCompletedAwaited(onCompleted); } private async Task FireOnCompletedAwaited(Stack, object>> onCompleted) @@ -755,7 +753,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } catch (Exception ex) { - ReportApplicationError(ex); + Log.ApplicationError(ConnectionId, TraceIdentifier, ex); } } } diff --git a/test/Kestrel.FunctionalTests/ResponseTests.cs b/test/Kestrel.FunctionalTests/ResponseTests.cs index 4f5513ec98..ca9616dfcf 100644 --- a/test/Kestrel.FunctionalTests/ResponseTests.cs +++ b/test/Kestrel.FunctionalTests/ResponseTests.cs @@ -146,7 +146,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests public async Task OnCompleteCalledEvenWhenOnStartingNotCalled() { var onStartingCalled = false; - var onCompletedCalled = false; + var onCompletedTcs = new TaskCompletionSource(); var hostBuilder = TransportSelector.GetWebHostBuilder() .UseKestrel() @@ -156,7 +156,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests app.Run(context => { context.Response.OnStarting(() => Task.Run(() => onStartingCalled = true)); - context.Response.OnCompleted(() => Task.Run(() => onCompletedCalled = true)); + context.Response.OnCompleted(() => Task.Run(() => + { + onCompletedTcs.SetResult(null); + })); // Prevent OnStarting call (see HttpProtocol.ProcessRequestsAsync()). throw new Exception(); @@ -173,7 +176,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); Assert.False(onStartingCalled); - Assert.True(onCompletedCalled); + await onCompletedTcs.Task.TimeoutAfter(TestConstants.DefaultTimeout); } } } @@ -294,6 +297,99 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests sendMalformedRequest: true); } + [Fact] + public async Task OnCompletedExceptionShouldNotPreventAResponse() + { + var hostBuilder = TransportSelector.GetWebHostBuilder() + .UseKestrel() + .UseUrls("http://127.0.0.1:0/") + .Configure(app => + { + app.Run(async context => + { + context.Response.OnCompleted(_ => throw new Exception(), null); + await context.Response.WriteAsync("hello, world"); + }); + }); + + using (var host = hostBuilder.Build()) + { + host.Start(); + + using (var client = new HttpClient()) + { + var response = await client.GetAsync($"http://127.0.0.1:{host.GetPort()}/"); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + } + } + + [Fact] + public async Task OnCompletedShouldNotBlockAResponse() + { + var delay = Task.Delay(TestConstants.DefaultTimeout); + var hostBuilder = TransportSelector.GetWebHostBuilder() + .UseKestrel() + .UseUrls("http://127.0.0.1:0/") + .Configure(app => + { + app.Run(async context => + { + context.Response.OnCompleted(async () => + { + await delay; + }); + await context.Response.WriteAsync("hello, world"); + }); + }); + + using (var host = hostBuilder.Build()) + { + host.Start(); + + using (var client = new HttpClient()) + { + var response = await client.GetAsync($"http://127.0.0.1:{host.GetPort()}/"); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.False(delay.IsCompleted); + } + } + } + + [Fact] + public async Task InvalidChunkedEncodingInRequestShouldNotBlockOnCompleted() + { + var onCompletedTcs = new TaskCompletionSource(); + + using (var server = new TestServer(httpContext => + { + httpContext.Response.OnCompleted(() => Task.Run(() => + { + onCompletedTcs.SetResult(null); + })); + return Task.CompletedTask; + })) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "GET / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "", + "gg"); + await connection.ReceiveForcedEnd( + "HTTP/1.1 200 OK", + $"Date: {server.Context.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + } + + await onCompletedTcs.Task.TimeoutAfter(TestConstants.DefaultTimeout); + } + private static async Task ResponseStatusCodeSetBeforeHttpContextDispose( RequestDelegate handler, HttpStatusCode? expectedClientStatusCode, @@ -1988,7 +2084,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests [Theory] [MemberData(nameof(ConnectionAdapterData))] - public async Task ThrowingInOnCompletedIsLoggedAndClosesConnection(ListenOptions listenOptions) + public async Task ThrowingInOnCompletedIsLogged(ListenOptions listenOptions) { var testContext = new TestServiceContext(); @@ -2024,7 +2120,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests "Host:", "", ""); - await connection.ReceiveForcedEnd( + await connection.ReceiveEnd( "HTTP/1.1 200 OK", $"Date: {testContext.DateHeaderValue}", "Content-Length: 11",