From 8e818e3549bcf95ada46911520638ccbec8fc900 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 15 Oct 2015 16:52:37 -0700 Subject: [PATCH] Address PR feedback --- .../Http/Frame.cs | 40 +++++++++--------- .../EngineTests.cs | 42 +++++++++++++++---- 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs index 22daa279a2..0d7b5ce6e5 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs @@ -42,7 +42,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Http private bool _responseStarted; private bool _keepAlive; private bool _autoChunk; - private bool _applicationFailed; + private Exception _applicationException; public Frame(ConnectionContext context) : base(context) { @@ -79,6 +79,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Http _responseStarted = false; _keepAlive = false; _autoChunk = false; + _applicationException = null; _requestHeaders.Reset(); ResetResponseHeaders(); @@ -183,10 +184,11 @@ namespace Microsoft.AspNet.Server.Kestrel.Http } finally { - // Trigger FireOnStarting if ProduceStart hasn't been called yet. - // We call it here, so it can go through our normal error handling - // and respond with a 500 if an OnStarting callback throws. - if (!_responseStarted) + // 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 (!_responseStarted && _applicationException == null) { await FireOnStarting(); } @@ -260,16 +262,16 @@ namespace Microsoft.AspNet.Server.Kestrel.Http } if (onStarting != null) { - foreach (var entry in onStarting) + try { - try + foreach (var entry in onStarting) { await entry.Key.Invoke(entry.Value); } - catch (Exception ex) - { - ReportApplicationError(ex); - } + } + catch (Exception ex) + { + ReportApplicationError(ex); } } } @@ -419,9 +421,11 @@ namespace Microsoft.AspNet.Server.Kestrel.Http await FireOnStarting(); - if (_applicationFailed) + if (_applicationException != null) { - throw new ObjectDisposedException(typeof(Frame).FullName); + throw new ObjectDisposedException( + "The response has been aborted due to an unhandled application exception.", + _applicationException); } await ProduceStart(immediate, appCompleted: false); @@ -444,12 +448,12 @@ namespace Microsoft.AspNet.Server.Kestrel.Http private async Task ProduceEnd() { - if (_applicationFailed) + if (_applicationException != null) { if (_responseStarted) { // We can no longer respond with a 500, so we simply close the connection. - _keepAlive = false; + _requestProcessingStopping = true; return; } else @@ -457,10 +461,6 @@ namespace Microsoft.AspNet.Server.Kestrel.Http StatusCode = 500; ReasonPhrase = null; - // If OnStarting hasn't been triggered yet, we don't want to trigger it now that - // the app func has failed. https://github.com/aspnet/KestrelHttpServer/issues/43 - _onStarting = null; - ResetResponseHeaders(); _responseHeaders.HeaderContentLength = "0"; } @@ -750,7 +750,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Http private void ReportApplicationError(Exception ex) { - _applicationFailed = true; + _applicationException = ex; Log.ApplicationError(ex); } } diff --git a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs index b5c36efce0..3aa7ca2105 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs @@ -550,7 +550,7 @@ namespace Microsoft.AspNet.Server.KestrelTests [MemberData(nameof(ConnectionFilterData))] public async Task ThrowingResultsIn500Response(ServiceContext testContext) { - var onStartingCallCount = 0; + bool onStartingCalled = false; var testLogger = new TestApplicationErrorLogger(); testContext.Log = new KestrelTrace(testLogger); @@ -560,7 +560,7 @@ namespace Microsoft.AspNet.Server.KestrelTests var response = frame.Get(); response.OnStarting(_ => { - onStartingCallCount++; + onStartingCalled = true; return Task.FromResult(null); }, null); @@ -597,7 +597,7 @@ namespace Microsoft.AspNet.Server.KestrelTests "", ""); - Assert.Equal(2, onStartingCallCount); + Assert.False(onStartingCalled); Assert.Equal(2, testLogger.ApplicationErrorsLogged); } } @@ -760,7 +760,8 @@ namespace Microsoft.AspNet.Server.KestrelTests [MemberData(nameof(ConnectionFilterData))] public async Task ThrowingInOnStartingResultsInFailedWritesAnd500Response(ServiceContext testContext) { - var onStartingCallCount = 0; + var onStartingCallCount1 = 0; + var onStartingCallCount2 = 0; var failedWriteCount = 0; var testLogger = new TestApplicationErrorLogger(); @@ -768,19 +769,28 @@ namespace Microsoft.AspNet.Server.KestrelTests using (var server = new TestServer(async frame => { + var onStartingException = new Exception(); + var response = frame.Get(); response.OnStarting(_ => { - onStartingCallCount++; - throw new Exception(); + onStartingCallCount1++; + throw onStartingException; + }, null); + response.OnStarting(_ => + { + onStartingCallCount2++; + throw onStartingException; }, null); response.Headers.Clear(); response.Headers["Content-Length"] = new[] { "11" }; - await Assert.ThrowsAsync(async () => + var writeException = await Assert.ThrowsAsync(async () => await response.Body.WriteAsync(Encoding.ASCII.GetBytes("Hello World"), 0, 11)); + Assert.Same(onStartingException, writeException.InnerException); + failedWriteCount++; }, testContext)) { @@ -811,7 +821,9 @@ namespace Microsoft.AspNet.Server.KestrelTests "", ""); - Assert.Equal(2, onStartingCallCount); + Assert.Equal(2, onStartingCallCount1); + // The second OnStarting callback should not be called since the first failed. + Assert.Equal(0, onStartingCallCount2); Assert.Equal(2, testLogger.ApplicationErrorsLogged); } } @@ -820,6 +832,9 @@ namespace Microsoft.AspNet.Server.KestrelTests [MemberData(nameof(ConnectionFilterData))] public async Task ThrowingInOnCompletedIsLoggedAndClosesConnection(ServiceContext testContext) { + var onCompletedCalled1 = false; + var onCompletedCalled2 = false; + var testLogger = new TestApplicationErrorLogger(); testContext.Log = new KestrelTrace(testLogger); @@ -828,6 +843,12 @@ namespace Microsoft.AspNet.Server.KestrelTests var response = frame.Get(); response.OnCompleted(_ => { + onCompletedCalled1 = true; + throw new Exception(); + }, null); + response.OnCompleted(_ => + { + onCompletedCalled2 = true; throw new Exception(); }, null); @@ -850,7 +871,10 @@ namespace Microsoft.AspNet.Server.KestrelTests "Hello World"); } - Assert.Equal(1, testLogger.ApplicationErrorsLogged); + // All OnCompleted callbacks should be called even if they throw. + Assert.Equal(2, testLogger.ApplicationErrorsLogged); + Assert.True(onCompletedCalled1); + Assert.True(onCompletedCalled2); } }