From ece8f33a658000cfc8f3bf9e333398e3a1de896d Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Tue, 23 Aug 2016 14:02:14 -0700 Subject: [PATCH] [Fixes #5175] Async resource filters' short circuited result getting executed more than once. --- .../Internal/ControllerActionInvoker.cs | 15 +- .../Internal/ControllerActionInvokerTest.cs | 151 +++++++++++++----- 2 files changed, 117 insertions(+), 49 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs index 3cf7d0003d..973167c80d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs @@ -357,7 +357,7 @@ private Task Next(ref State next, ref Scope scope, ref object state, ref bool is goto case State.ExceptionBegin; } } - + case State.ResourceAsyncBegin: { Debug.Assert(state != null); @@ -392,13 +392,14 @@ private Task Next(ref State next, ref Scope scope, ref object state, ref bool is Canceled = true, Result = _resourceExecutingContext.Result, }; - } - _diagnosticSource.AfterOnResourceExecution(_resourceExecutedContext, filter); + _diagnosticSource.AfterOnResourceExecution(_resourceExecutedContext, filter); - if (_resourceExecutingContext.Result != null) - { - goto case State.ResourceShortCircuit; + // A filter could complete a Task without setting a result + if (_resourceExecutingContext.Result != null) + { + goto case State.ResourceShortCircuit; + } } goto case State.ResourceEnd; @@ -1222,7 +1223,7 @@ private async Task InvokeActionMethodAsync() arguments, controller); logger.ActionMethodExecuting(controllerContext, orderedArguments); - + var returnType = executor.MethodReturnType; if (returnType == typeof(void)) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs index 7ff95e85d4..f847a4cdb1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs @@ -82,31 +82,41 @@ public async Task InvokeAction_InvokesExceptionFilter_WhenActionThrows() { // Arrange Exception exception = null; - IActionResult result = null; + IActionResult resultFromAction = null; + var expected = new Mock(MockBehavior.Strict); + expected + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); - var filter = new Mock(MockBehavior.Strict); - filter + var filter1 = new Mock(MockBehavior.Strict); + filter1 + .Setup(f => f.OnException(It.IsAny())) + .Verifiable(); + var filter2 = new Mock(MockBehavior.Strict); + filter2 .Setup(f => f.OnException(It.IsAny())) .Callback(context => { exception = context.Exception; - result = context.Result; + resultFromAction = context.Result; // Handle the exception - context.Result = new EmptyResult(); + context.Result = expected.Object; }) .Verifiable(); - var invoker = CreateInvoker(filter.Object, actionThrows: true); + var invoker = CreateInvoker(new[] { filter1.Object, filter2.Object }, actionThrows: true); // Act await invoker.InvokeAsync(); // Assert - filter.Verify(f => f.OnException(It.IsAny()), Times.Once()); + expected.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + filter2.Verify(f => f.OnException(It.IsAny()), Times.Once()); Assert.Same(_actionException, exception); - Assert.Null(result); + Assert.Null(resultFromAction); } [Fact] @@ -114,34 +124,45 @@ public async Task InvokeAction_InvokesAsyncExceptionFilter_WhenActionThrows() { // Arrange Exception exception = null; - IActionResult result = null; + IActionResult resultFromAction = null; + var expected = new Mock(MockBehavior.Strict); + expected + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); - var filter = new Mock(MockBehavior.Strict); - filter + var filter1 = new Mock(MockBehavior.Strict); + filter1 + .Setup(f => f.OnExceptionAsync(It.IsAny())) + .Returns((context) => Task.FromResult(true)) + .Verifiable(); + var filter2 = new Mock(MockBehavior.Strict); + filter2 .Setup(f => f.OnExceptionAsync(It.IsAny())) .Callback(context => { exception = context.Exception; - result = context.Result; + resultFromAction = context.Result; // Handle the exception - context.Result = new EmptyResult(); + context.Result = expected.Object; }) .Returns((context) => Task.FromResult(true)) .Verifiable(); - var invoker = CreateInvoker(filter.Object, actionThrows: true); + var invoker = CreateInvoker(new[] { filter1.Object, filter2.Object }, actionThrows: true); // Act await invoker.InvokeAsync(); // Assert - filter.Verify( + expected.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); + filter2.Verify( f => f.OnExceptionAsync(It.IsAny()), Times.Once()); Assert.Same(_actionException, exception); - Assert.Null(result); + Assert.Null(resultFromAction); } [Fact] @@ -340,22 +361,33 @@ public async Task InvokeAction_InvokesAsyncAuthorizationFilter() public async Task InvokeAction_InvokesAuthorizationFilter_ShortCircuit() { // Arrange - var challenge = new Mock(MockBehavior.Loose).Object; + var challenge = new Mock(MockBehavior.Strict); + challenge + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); var filter1 = new Mock(MockBehavior.Strict); filter1 .Setup(f => f.OnAuthorization(It.IsAny())) - .Callback(c => c.Result = challenge) + .Callback(c => Task.FromResult(true)) .Verifiable(); var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnAuthorization(It.IsAny())) + .Callback(c => c.Result = challenge.Object) + .Verifiable(); - var invoker = CreateInvoker(new[] { filter1.Object, filter2.Object }); + var filter3 = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker(new[] { filter1.Object, filter2.Object, filter3.Object }); // Act await invoker.InvokeAsync(); // Assert + challenge.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); filter1.Verify(f => f.OnAuthorization(It.IsAny()), Times.Once()); Assert.False(invoker.ControllerFactory.CreateCalled); } @@ -364,26 +396,39 @@ public async Task InvokeAction_InvokesAuthorizationFilter_ShortCircuit() public async Task InvokeAction_InvokesAsyncAuthorizationFilter_ShortCircuit() { // Arrange - var challenge = new Mock(MockBehavior.Loose).Object; + var challenge = new Mock(MockBehavior.Strict); + challenge + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); var filter1 = new Mock(MockBehavior.Strict); filter1 .Setup(f => f.OnAuthorizationAsync(It.IsAny())) .Returns((context) => { - context.Result = challenge; return Task.FromResult(true); }) .Verifiable(); - var filter2 = new Mock(MockBehavior.Strict); + var filter2 = new Mock(MockBehavior.Strict); + filter2 + .Setup(f => f.OnAuthorizationAsync(It.IsAny())) + .Returns((context) => + { + context.Result = challenge.Object; + return Task.FromResult(true); + }); - var invoker = CreateInvoker(new IFilterMetadata[] { filter1.Object, filter2.Object }); + var filter3 = new Mock(MockBehavior.Strict); + + var invoker = CreateInvoker(new IFilterMetadata[] { filter1.Object, filter2.Object, filter3.Object }); // Act await invoker.InvokeAsync(); // Assert + challenge.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); filter1.Verify( f => f.OnAuthorizationAsync(It.IsAny()), Times.Once()); @@ -392,7 +437,7 @@ public async Task InvokeAction_InvokesAsyncAuthorizationFilter_ShortCircuit() } [Fact] - public async Task InvokeAction_ExceptionInAuthorizationFilterCannotBeHandledByOtherFilters() + public async Task InvokeAction_ExceptionInAuthorizationFilter_CannotBeHandledByOtherFilters() { // Arrange var expected = new InvalidCastException(); @@ -524,7 +569,11 @@ public async Task InvokeAction_InvokesAsyncActionFilter() public async Task InvokeAction_InvokesActionFilter_ShortCircuit() { // Arrange - var result = new EmptyResult(); + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); ActionExecutedContext context = null; @@ -538,7 +587,7 @@ public async Task InvokeAction_InvokesActionFilter_ShortCircuit() var actionFilter2 = new Mock(MockBehavior.Strict); actionFilter2 .Setup(f => f.OnActionExecuting(It.IsAny())) - .Callback(c => c.Result = result) + .Callback(c => c.Result = result.Object) .Verifiable(); var actionFilter3 = new Mock(MockBehavior.Strict); @@ -559,23 +608,29 @@ public async Task InvokeAction_InvokesActionFilter_ShortCircuit() await invoker.InvokeAsync(); // Assert + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); actionFilter1.Verify(f => f.OnActionExecuting(It.IsAny()), Times.Once()); actionFilter1.Verify(f => f.OnActionExecuted(It.IsAny()), Times.Once()); actionFilter2.Verify(f => f.OnActionExecuting(It.IsAny()), Times.Once()); + actionFilter2.Verify(f => f.OnActionExecuted(It.IsAny()), Times.Never()); resultFilter.Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); resultFilter.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); Assert.True(context.Canceled); - Assert.Same(context.Result, result); + Assert.Same(context.Result, result.Object); } [Fact] public async Task InvokeAction_InvokesAsyncActionFilter_ShortCircuit_WithResult() { // Arrange - var result = new EmptyResult(); + var result = new Mock(MockBehavior.Strict); + result + .Setup(r => r.ExecuteResultAsync(It.IsAny())) + .Returns(Task.FromResult(true)) + .Verifiable(); ActionExecutedContext context = null; @@ -592,29 +647,34 @@ public async Task InvokeAction_InvokesAsyncActionFilter_ShortCircuit_WithResult( .Returns((c, next) => { // Notice we're not calling next - c.Result = result; + c.Result = result.Object; return Task.FromResult(true); }) .Verifiable(); var actionFilter3 = new Mock(MockBehavior.Strict); - var resultFilter = new Mock(MockBehavior.Strict); - resultFilter.Setup(f => f.OnResultExecuting(It.IsAny())).Verifiable(); - resultFilter.Setup(f => f.OnResultExecuted(It.IsAny())).Verifiable(); + var resultFilter1 = new Mock(MockBehavior.Strict); + resultFilter1.Setup(f => f.OnResultExecuting(It.IsAny())).Verifiable(); + resultFilter1.Setup(f => f.OnResultExecuted(It.IsAny())).Verifiable(); + var resultFilter2 = new Mock(MockBehavior.Strict); + resultFilter2.Setup(f => f.OnResultExecuting(It.IsAny())).Verifiable(); + resultFilter2.Setup(f => f.OnResultExecuted(It.IsAny())).Verifiable(); var invoker = CreateInvoker(new IFilterMetadata[] { actionFilter1.Object, actionFilter2.Object, actionFilter3.Object, - resultFilter.Object, + resultFilter1.Object, + resultFilter2.Object, }); // Act await invoker.InvokeAsync(); // Assert + result.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); actionFilter1.Verify(f => f.OnActionExecuting(It.IsAny()), Times.Once()); actionFilter1.Verify(f => f.OnActionExecuted(It.IsAny()), Times.Once()); @@ -622,11 +682,13 @@ public async Task InvokeAction_InvokesAsyncActionFilter_ShortCircuit_WithResult( f => f.OnActionExecutionAsync(It.IsAny(), It.IsAny()), Times.Once()); - resultFilter.Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); - resultFilter.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); + resultFilter1.Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + resultFilter1.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); + resultFilter2.Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + resultFilter2.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); Assert.True(context.Canceled); - Assert.Same(context.Result, result); + Assert.Same(context.Result, result.Object); } [Fact] @@ -766,10 +828,9 @@ public async Task InvokeAction_InvokesActionFilter_WithExceptionThrownByActionFi .Verifiable(); var filter2 = new Mock(MockBehavior.Strict); - filter2.Setup(f => f.OnActionExecuting(It.IsAny())).Verifiable(); filter2 - .Setup(f => f.OnActionExecuted(It.IsAny())) - .Callback(c => { throw exception; }) + .Setup(f => f.OnActionExecuting(It.IsAny())) + .Callback(c => { throw exception; }) .Verifiable(); var invoker = CreateInvoker(new[] { filter1.Object, filter2.Object }); @@ -782,6 +843,7 @@ public async Task InvokeAction_InvokesActionFilter_WithExceptionThrownByActionFi filter1.Verify(f => f.OnActionExecuted(It.IsAny()), Times.Once()); filter2.Verify(f => f.OnActionExecuting(It.IsAny()), Times.Once()); + filter2.Verify(f => f.OnActionExecuted(It.IsAny()), Times.Never()); Assert.Same(exception, context.Exception); Assert.Null(context.Result); @@ -858,7 +920,9 @@ public async Task InvokeAction_InvokesActionFilter_HandleException() resultFilter.Setup(f => f.OnResultExecuting(It.IsAny())).Verifiable(); resultFilter.Setup(f => f.OnResultExecuted(It.IsAny())).Verifiable(); - var invoker = CreateInvoker(new IFilterMetadata[] { actionFilter.Object, resultFilter.Object }, actionThrows: true); + var invoker = CreateInvoker( + new IFilterMetadata[] { actionFilter.Object, resultFilter.Object }, + actionThrows: true); // Act await invoker.InvokeAsync(); @@ -913,7 +977,7 @@ public async Task InvokeAction_InvokesAsyncResultFilter() } [Fact] - public async Task InvokeAction_InvokesResultFilter_ShortCircuit() + public async Task InvokeAction_InvokesResultFilter_ShortCircuit_WithCancel() { // Arrange ResultExecutedContext context = null; @@ -947,6 +1011,7 @@ public async Task InvokeAction_InvokesResultFilter_ShortCircuit() filter1.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); filter2.Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + filter2.Verify(f => f.OnResultExecuted(It.IsAny()), Times.Never()); Assert.True(context.Canceled); } @@ -1797,6 +1862,7 @@ public async Task InvokeAction_InvokesAsyncResourceFilter_ShortCircuit() await invoker.InvokeAsync(); // Assert + expected.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); Assert.Same(expected.Object, context.Result); Assert.True(context.Canceled); Assert.False(invoker.ControllerFactory.CreateCalled); @@ -1899,6 +1965,7 @@ public async Task InvokeAction_InvokesResourceFilter_ShortCircuit() await invoker.InvokeAsync(); // Assert + expected.Verify(r => r.ExecuteResultAsync(It.IsAny()), Times.Once()); Assert.Same(expected.Object, context.Result); Assert.True(context.Canceled); Assert.False(invoker.ControllerFactory.CreateCalled); @@ -2751,7 +2818,7 @@ public async Task Invoke_WritesDiagnostic_ActionInvoked() Assert.NotNull(listener.AfterAction?.ActionDescriptor); Assert.NotNull(listener.AfterAction?.HttpContext); } - + public async Task InvokeAction_ExceptionBubbling_AsyncActionFilter_To_ResourceFilter() { // Arrange