From f1071dea50d03b4bad0577aa402811f92b3471b5 Mon Sep 17 00:00:00 2001 From: Mike Harder Date: Tue, 19 Jul 2016 12:25:55 -0700 Subject: [PATCH] Set StatusCode before disposing HttpContext (#876) --- .../Internal/Http/Frame.cs | 1 - .../Internal/Http/FrameOfT.cs | 98 ++++++++------ .../ResponseTests.cs | 120 ++++++++++++++++++ 3 files changed, 176 insertions(+), 43 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs index e31bd4777a..e2c0d88b68 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Globalization; using System.IO; using System.Linq; using System.Net; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs index f2b0dac367..b71f94dbd3 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs @@ -92,55 +92,69 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http var context = _application.CreateContext(this); try { - await _application.ProcessRequestAsync(context).ConfigureAwait(false); - VerifyResponseContentLength(); - } - catch (Exception ex) - { - ReportApplicationError(ex); + try + { + await _application.ProcessRequestAsync(context).ConfigureAwait(false); + VerifyResponseContentLength(); + } + catch (Exception ex) + { + ReportApplicationError(ex); + } + finally + { + // Trigger OnStarting if it hasn't been called yet and the app hasn't + // already failed. If an OnStarting callback throws we can go through + // our normal error handling in ProduceEnd. + // https://github.com/aspnet/KestrelHttpServer/issues/43 + if (!HasResponseStarted && _applicationException == null && _onStarting != null) + { + await FireOnStarting(); + } + + PauseStreams(); + + if (_onCompleted != null) + { + await FireOnCompleted(); + } + + // If _requestAbort is set, the connection has already been closed. + if (Volatile.Read(ref _requestAborted) == 0) + { + ResumeStreams(); + + if (_keepAlive) + { + // Finish reading the request body in case the app did not. + await messageBody.Consume(); + } + + // ProduceEnd() must be called before _application.DisposeContext(), to ensure + // HttpContext.Response.StatusCode is correctly set when + // IHttpContextFactory.Dispose(HttpContext) is called. + await ProduceEnd(); + } + else if (!HasResponseStarted) + { + // If the request was aborted and no response was sent, there's no + // meaningful status code to log. + StatusCode = 0; + } + } } finally { - // Trigger OnStarting if it hasn't been called yet and the app hasn't - // already failed. If an OnStarting callback throws we can go through - // our normal error handling in ProduceEnd. - // https://github.com/aspnet/KestrelHttpServer/issues/43 - if (!HasResponseStarted && _applicationException == null && _onStarting != null) - { - await FireOnStarting(); - } - - PauseStreams(); - - if (_onCompleted != null) - { - await FireOnCompleted(); - } - _application.DisposeContext(context, _applicationException); } + } - // If _requestAbort is set, the connection has already been closed. - if (Volatile.Read(ref _requestAborted) == 0) - { - ResumeStreams(); + StopStreams(); - if (_keepAlive) - { - // Finish reading the request body in case the app did not. - await messageBody.Consume(); - } - - await ProduceEnd(); - } - - StopStreams(); - - if (!_keepAlive) - { - // End the connection for non keep alive as data incoming may have been thrown off - return; - } + if (!_keepAlive) + { + // End the connection for non keep alive as data incoming may have been thrown off + return; } // Don't reset frame state if we're exiting the loop. This avoids losing request rejection diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs index 719b61cab1..336be582ee 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs @@ -6,14 +6,17 @@ using System.IO; using System.Linq; using System.Net; using System.Net.Http; +using System.Net.Sockets; using System.Text; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure; using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; @@ -185,6 +188,123 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests } } + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposeAppException() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + context => + { + throw new Exception(); + }, + expectedClientStatusCode: HttpStatusCode.InternalServerError, + expectedServerStatusCode: HttpStatusCode.InternalServerError); + } + + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposeRequestAborted() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + context => + { + context.Abort(); + return TaskCache.CompletedTask; + }, + expectedClientStatusCode: null, + expectedServerStatusCode: 0); + } + + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposeRequestAbortedAppException() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + context => + { + context.Abort(); + throw new Exception(); + }, + expectedClientStatusCode: null, + expectedServerStatusCode: 0); + } + + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformed() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + context => + { + return TaskCache.CompletedTask; + }, + expectedClientStatusCode: null, + expectedServerStatusCode: HttpStatusCode.BadRequest, + sendMalformedRequest: true); + } + + private static async Task ResponseStatusCodeSetBeforeHttpContextDispose( + RequestDelegate handler, + HttpStatusCode? expectedClientStatusCode, + HttpStatusCode expectedServerStatusCode, + bool sendMalformedRequest = false) + { + var mockHttpContextFactory = new Mock(); + mockHttpContextFactory.Setup(f => f.Create(It.IsAny())) + .Returns(fc => new DefaultHttpContext(fc)); + + var disposedTcs = new TaskCompletionSource(); + mockHttpContextFactory.Setup(f => f.Dispose(It.IsAny())) + .Callback(c => + { + disposedTcs.TrySetResult(c.Response.StatusCode); + }); + + var hostBuilder = new WebHostBuilder() + .UseKestrel() + .UseUrls("http://127.0.0.1:0") + .ConfigureServices(services => services.AddSingleton(mockHttpContextFactory.Object)) + .Configure(app => + { + app.Run(handler); + }); + + using (var host = hostBuilder.Build()) + { + host.Start(); + + if (!sendMalformedRequest) + { + using (var client = new HttpClient()) + { + try + { + var response = await client.GetAsync($"http://localhost:{host.GetPort()}/"); + Assert.Equal(expectedClientStatusCode, response.StatusCode); + } + catch + { + if (expectedClientStatusCode != null) + { + throw; + } + } + } + } + else + { + using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(new IPEndPoint(IPAddress.Loopback, host.GetPort())); + socket.Send(Encoding.ASCII.GetBytes( + "POST / HTTP/1.1\r\n" + + "Transfer-Encoding: chunked\r\n" + + "\r\n" + + "wrong")); + } + } + + var disposedStatusCode = await disposedTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); + Assert.Equal(expectedServerStatusCode, (HttpStatusCode)disposedStatusCode); + } + } + // https://github.com/aspnet/KestrelHttpServer/pull/1111/files#r80584475 explains the reason for this test. [Fact] public async Task SingleErrorResponseSentWhenAppSwallowsBadRequestException()