diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs index 973167c80d..844fc218e0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs @@ -577,6 +577,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (exceptionContext.Exception == null || exceptionContext.ExceptionHandled) { + // We don't need to do anthing to trigger a short circuit. If there's another + // exception filter on the stack it will check the same set of conditions + // and then just skip itself. _logger.ExceptionFilterShortCircuited(filter); } @@ -614,6 +617,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (exceptionContext.Exception == null || exceptionContext.ExceptionHandled) { + // We don't need to do anthing to trigger a short circuit. If there's another + // exception filter on the stack it will check the same set of conditions + // and then just skip itself. _logger.ExceptionFilterShortCircuited(filter); } } @@ -626,11 +632,23 @@ namespace Microsoft.AspNetCore.Mvc.Internal goto case State.ActionBegin; } - case State.ExceptionShortCircuit: + case State.ExceptionHandled: { + // We arrive in this state when an exception happened, but was handled by exception filters + // either by setting ExceptionHandled, or nulling out the Exception or setting a result + // on the ExceptionContext. + // + // We need to execute the result (if any) and then exit gracefully which unwinding Resource + // filters. + Debug.Assert(state != null); Debug.Assert(_exceptionContext != null); + if (_exceptionContext.Result == null) + { + _exceptionContext.Result = new EmptyResult(); + } + Task task; if (scope == Scope.Resource) { @@ -657,6 +675,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal return task; } + // Found this while investigating #5594. This is not right, but we think it's harmless + // so we're leaving it here for the patch release. This should go to InvokeEnd if the + // scope is not Resource because that can only happen when there are no resource filters. goto case State.ResourceEnd; } @@ -672,12 +693,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (exceptionContext != null) { - if (exceptionContext.Result != null && !exceptionContext.ExceptionHandled) + if (exceptionContext.Result != null || + exceptionContext.Exception == null || + exceptionContext.ExceptionHandled) { - goto case State.ExceptionShortCircuit; + goto case State.ExceptionHandled; } Rethrow(exceptionContext); + Debug.Fail("unreachable"); } goto case State.ResultBegin; @@ -1488,7 +1512,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal ExceptionSyncBegin, ExceptionSyncEnd, ExceptionInside, - ExceptionShortCircuit, + ExceptionHandled, ExceptionEnd, ActionBegin, ActionNext, diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs index 52c050cbde..7e8f258098 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs @@ -166,7 +166,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull() + public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull_WithoutResult() { // Arrange var filter1 = new Mock(MockBehavior.Strict); @@ -193,7 +193,47 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled() + public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull_WithResult() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + + var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnException(It.IsAny())) + .Callback(context => + { + context.Result = result.Object; + context.Exception = null; + }) + .Verifiable(); + + // Result filters are never used when an exception bubbles up to exception filters. + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter2.Verify( + f => f.OnException(It.IsAny()), + Times.Once()); + + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + + [Fact] + public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled_WithoutResult() { // Arrange var filter1 = new Mock(MockBehavior.Strict); @@ -219,7 +259,47 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull() + public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled_WithResult() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + + var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnException(It.IsAny())) + .Callback(context => + { + context.Result = result.Object; + context.ExceptionHandled = true; + }) + .Verifiable(); + + // Result filters are never used when an exception bubbles up to exception filters. + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter2.Verify( + f => f.OnException(It.IsAny()), + Times.Once()); + + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + + [Fact] + public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull_WithoutResult() { // Arrange var filter1 = new Mock(MockBehavior.Strict); @@ -229,7 +309,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal .Setup(f => f.OnExceptionAsync(It.IsAny())) .Callback(context => { - filter2.ToString(); context.Exception = null; }) .Returns((context) => Task.FromResult(true)) @@ -248,7 +327,48 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled() + public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull_WithResult() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + var filter2 = new Mock(MockBehavior.Strict); + + filter2 + .Setup(f => f.OnExceptionAsync(It.IsAny())) + .Callback(context => + { + context.Exception = null; + context.Result = result.Object; + }) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + + // Result filters are never used when an exception bubbles up to exception filters. + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter2.Verify( + f => f.OnExceptionAsync(It.IsAny()), + Times.Once()); + + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + + [Fact] + public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled_WithoutResult() { // Arrange var filter1 = new Mock(MockBehavior.Strict); @@ -274,6 +394,88 @@ namespace Microsoft.AspNetCore.Mvc.Internal Times.Once()); } + [Fact] + public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled_WithResult() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + var filter2 = new Mock(MockBehavior.Strict); + + filter2 + .Setup(f => f.OnExceptionAsync(It.IsAny())) + .Callback(context => + { + context.ExceptionHandled = true; + context.Result = result.Object; + }) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + + // Result filters are never used when an exception bubbles up to exception filters. + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter2.Verify( + f => f.OnExceptionAsync(It.IsAny()), + Times.Once()); + + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + + [Fact] + public async Task InvokeAction_InvokesAsyncExceptionFilter_SettingResultDoesNotShortCircuit() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + filter1 + .Setup(f => f.OnExceptionAsync(It.IsAny())) + .Callback(context => + { + context.Result = result.Object; + }) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + + var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnException(It.IsAny())) + .Callback(c => { }) // Does nothing, we just want to verify that it was called. + .Verifiable(); + + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter1.Verify(f => f.OnExceptionAsync(It.IsAny()), Times.Once()); + filter2.Verify(f => f.OnException(It.IsAny()), Times.Once()); + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + [Fact] public async Task InvokeAction_InvokesExceptionFilter_UnhandledExceptionIsThrown() { @@ -320,6 +522,43 @@ namespace Microsoft.AspNetCore.Mvc.Internal result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); } + [Fact] + public async Task InvokeAction_InvokesExceptionFilter_SettingResultDoesNotShortCircuit() + { + // Arrange + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + + var filter1 = new Mock(MockBehavior.Strict); + filter1 + .Setup(f => f.OnException(It.IsAny())) + .Callback(c => c.Result = result.Object) + .Verifiable(); + + var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnException(It.IsAny())) + .Callback(c => { }) // Does nothing, we just want to verify that it was called. + .Verifiable(); + + var resultFilter = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker( + new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object }, + actionThrows: true); + + // Act + await invoker.InvokeAsync(); + + // Assert + filter1.Verify(f => f.OnException(It.IsAny()), Times.Once()); + filter2.Verify(f => f.OnException(It.IsAny()), Times.Once()); + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + } + [Fact] public async Task InvokeAction_InvokesAuthorizationFilter() {