From 71dd3d64245b7c1a013fa872ee6dc1649b8f64c0 Mon Sep 17 00:00:00 2001 From: John Luo Date: Wed, 19 Aug 2020 17:41:09 -0700 Subject: [PATCH 1/2] Throw original exception if exception handler is not found --- .../ExceptionHandlerMiddleware.cs | 12 ++- .../test/UnitTests/ExceptionHandlerTest.cs | 85 ++++++++++++++++++- 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs index 65564cbd5926..5479aa02d69e 100644 --- a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs +++ b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs @@ -113,11 +113,21 @@ private async Task HandleException(HttpContext context, ExceptionDispatchInfo ed }; context.Features.Set(exceptionHandlerFeature); context.Features.Set(exceptionHandlerFeature); - context.Response.StatusCode = 500; + context.Response.StatusCode = StatusCodes.Status500InternalServerError; context.Response.OnStarting(_clearCacheHeadersDelegate, context.Response); await _options.ExceptionHandler(context); + if (context.Response.StatusCode == StatusCodes.Status404NotFound) + { + // Reset status and clear headers + context.Response.StatusCode = StatusCodes.Status500InternalServerError; + await ClearCacheHeaders(context.Response); + + // This exception will be logged and the original exception wil be thrown + throw new InvalidOperationException($"No exception handler was found at the configured path {_options.ExceptionHandlingPath}."); + } + if (_diagnosticListener.IsEnabled() && _diagnosticListener.IsEnabled("Microsoft.AspNetCore.Diagnostics.HandledException")) { _diagnosticListener.Write("Microsoft.AspNetCore.Diagnostics.HandledException", new { httpContext = context, exception = edi.SourceException }); diff --git a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs index c09bc6676826..e438546b439b 100644 --- a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs +++ b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs @@ -7,17 +7,17 @@ using System.IO; using System.Linq; using System.Net; -using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.TestHost; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Testing; using Xunit; namespace Microsoft.AspNetCore.Diagnostics @@ -469,5 +469,86 @@ public void UsingExceptionHandler_ThrowsAnException_WhenExceptionHandlingPathNot "Alternatively, set one of the aforementioned properties in 'Startup.ConfigureServices' as follows: 'services.AddExceptionHandler(options => { ... });'.", exception.Message); } + + [Fact] + public async Task ExceptionHandlerNotFound_RethrowsOriginalError() + { + var sink = new TestSink(TestSink.EnableWithTypeName); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + + using var host = new HostBuilder() + .ConfigureWebHost(webHostBuilder => + { + webHostBuilder + .UseTestServer() + .ConfigureServices(services => + { + services.AddSingleton(loggerFactory); + }) + .Configure(app => + { + app.Use(async (httpContext, next) => + { + Exception exception = null; + try + { + await next(); + } + catch (InvalidOperationException ex) + { + exception = ex; + } + + // The original exception is thrown + Assert.NotNull(exception); + Assert.Equal("Something bad happened.", exception.Message); + }); + + app.UseExceptionHandler("/non-existent-hander"); + + app.Map("/handle-errors", (innerAppBuilder) => + { + innerAppBuilder.Run(async (httpContext) => + { + await httpContext.Response.WriteAsync("Handled error in a custom way."); + }); + }); + + app.Map("/throw", (innerAppBuilder) => + { + innerAppBuilder.Run(httpContext => + { + throw new InvalidOperationException("Something bad happened."); + }); + }); + }); + }).Build(); + + await host.StartAsync(); + + using (var server = host.GetTestServer()) + { + var client = server.CreateClient(); + var response = await client.GetAsync("throw"); + Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + Assert.Equal(string.Empty, await response.Content.ReadAsStringAsync()); + IEnumerable values; + Assert.True(response.Headers.CacheControl.NoCache); + Assert.True(response.Headers.CacheControl.NoStore); + Assert.True(response.Headers.TryGetValues("Pragma", out values)); + Assert.Single(values); + Assert.Equal("no-cache", values.First()); + Assert.True(response.Content.Headers.TryGetValues("Expires", out values)); + Assert.Single(values); + Assert.Equal("-1", values.First()); + Assert.False(response.Headers.TryGetValues("ETag", out values)); + } + + Assert.Contains(sink.Writes, w => + w.LogLevel == LogLevel.Error + && w.Message == "An exception was thrown attempting to execute the error handler." + && w.Exception != null + && w.Exception.Message == "No exception handler was found at the configured path /non-existent-hander."); + } } } From 5a0acd85b01de5559c8b977be72d10fcc38a3a23 Mon Sep 17 00:00:00 2001 From: John Luo Date: Wed, 19 Aug 2020 21:56:35 -0700 Subject: [PATCH 2/2] Feedback --- .../src/DiagnosticsLoggerExtensions.cs | 10 ++++++++- .../ExceptionHandlerMiddleware.cs | 20 +++++++----------- .../test/UnitTests/ExceptionHandlerTest.cs | 21 +++++++------------ 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/Middleware/Diagnostics/src/DiagnosticsLoggerExtensions.cs b/src/Middleware/Diagnostics/src/DiagnosticsLoggerExtensions.cs index 995bbc6786ad..fdd67aee9e0e 100644 --- a/src/Middleware/Diagnostics/src/DiagnosticsLoggerExtensions.cs +++ b/src/Middleware/Diagnostics/src/DiagnosticsLoggerExtensions.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -19,6 +19,9 @@ internal static class DiagnosticsLoggerExtensions private static readonly Action _errorHandlerException = LoggerMessage.Define(LogLevel.Error, new EventId(3, "Exception"), "An exception was thrown attempting to execute the error handler."); + private static readonly Action _errorHandlerNotFound = + LoggerMessage.Define(LogLevel.Warning, new EventId(4, "HandlerNotFound"), "No exception handler was found, rethrowing original exception."); + // DeveloperExceptionPageMiddleware private static readonly Action _responseStartedErrorPageMiddleware = LoggerMessage.Define(LogLevel.Warning, new EventId(2, "ResponseStarted"), "The response has already started, the error page middleware will not be executed."); @@ -41,6 +44,11 @@ public static void ErrorHandlerException(this ILogger logger, Exception exceptio _errorHandlerException(logger, exception); } + public static void ErrorHandlerNotFound(this ILogger logger) + { + _errorHandlerNotFound(logger, null); + } + public static void ResponseStartedErrorPageMiddleware(this ILogger logger) { _responseStartedErrorPageMiddleware(logger, null); diff --git a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs index 5479aa02d69e..fbf0c8913d55 100644 --- a/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs +++ b/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs @@ -118,23 +118,17 @@ private async Task HandleException(HttpContext context, ExceptionDispatchInfo ed await _options.ExceptionHandler(context); - if (context.Response.StatusCode == StatusCodes.Status404NotFound) + if (context.Response.StatusCode != StatusCodes.Status404NotFound) { - // Reset status and clear headers - context.Response.StatusCode = StatusCodes.Status500InternalServerError; - await ClearCacheHeaders(context.Response); + if (_diagnosticListener.IsEnabled() && _diagnosticListener.IsEnabled("Microsoft.AspNetCore.Diagnostics.HandledException")) + { + _diagnosticListener.Write("Microsoft.AspNetCore.Diagnostics.HandledException", new { httpContext = context, exception = edi.SourceException }); + } - // This exception will be logged and the original exception wil be thrown - throw new InvalidOperationException($"No exception handler was found at the configured path {_options.ExceptionHandlingPath}."); + return; } - if (_diagnosticListener.IsEnabled() && _diagnosticListener.IsEnabled("Microsoft.AspNetCore.Diagnostics.HandledException")) - { - _diagnosticListener.Write("Microsoft.AspNetCore.Diagnostics.HandledException", new { httpContext = context, exception = edi.SourceException }); - } - - // TODO: Optional re-throw? We'll re-throw the original exception by default if the error handler throws. - return; + _logger.ErrorHandlerNotFound(); } catch (Exception ex2) { diff --git a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs index e438546b439b..52dec57e343d 100644 --- a/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs +++ b/src/Middleware/Diagnostics/test/UnitTests/ExceptionHandlerTest.cs @@ -497,11 +497,15 @@ public async Task ExceptionHandlerNotFound_RethrowsOriginalError() catch (InvalidOperationException ex) { exception = ex; + + // This mimics what the server would do when an exception occurs + httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError; } // The original exception is thrown Assert.NotNull(exception); Assert.Equal("Something bad happened.", exception.Message); + }); app.UseExceptionHandler("/non-existent-hander"); @@ -532,23 +536,12 @@ public async Task ExceptionHandlerNotFound_RethrowsOriginalError() var response = await client.GetAsync("throw"); Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); Assert.Equal(string.Empty, await response.Content.ReadAsStringAsync()); - IEnumerable values; - Assert.True(response.Headers.CacheControl.NoCache); - Assert.True(response.Headers.CacheControl.NoStore); - Assert.True(response.Headers.TryGetValues("Pragma", out values)); - Assert.Single(values); - Assert.Equal("no-cache", values.First()); - Assert.True(response.Content.Headers.TryGetValues("Expires", out values)); - Assert.Single(values); - Assert.Equal("-1", values.First()); - Assert.False(response.Headers.TryGetValues("ETag", out values)); } Assert.Contains(sink.Writes, w => - w.LogLevel == LogLevel.Error - && w.Message == "An exception was thrown attempting to execute the error handler." - && w.Exception != null - && w.Exception.Message == "No exception handler was found at the configured path /non-existent-hander."); + w.LogLevel == LogLevel.Warning + && w.EventId == 4 + && w.Message == "No exception handler was found, rethrowing original exception."); } } }