From 652e33087555638b8d156cc47f0f4e1d6af0e856 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 13 May 2016 08:53:56 -0700 Subject: [PATCH] Fix #4631 and avoid Task creation in invoker This change corrects and ordering bug between the creation of the 'context' and the diagnostic source event that occurs before a synchronous filter's 'after' stage. Also made some simple changes to avoid allocating Task in many common cases. Now we'll only create the Task when we really need it (async filters). --- .../Internal/FilterActionInvoker.cs | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs index 88092134bb..b3be1f9d99 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs @@ -227,7 +227,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal return InvokeResourceFilterAsync(); } - private async Task InvokeResourceFilterAsync() + private async Task InvokeResourceFilterAwaitedAsync() { Debug.Assert(_resourceExecutingContext != null); @@ -243,6 +243,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal throw new InvalidOperationException(message); } + await InvokeResourceFilterAsync(); + + Debug.Assert(_resourceExecutedContext != null); + return _resourceExecutedContext; + } + + private async Task InvokeResourceFilterAsync() + { + Debug.Assert(_resourceExecutingContext != null); + var item = _cursor.GetNextFilter(); try { @@ -250,7 +260,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal { _diagnosticSource.BeforeOnResourceExecution(_resourceExecutingContext, item.FilterAsync); - await item.FilterAsync.OnResourceExecutionAsync(_resourceExecutingContext, InvokeResourceFilterAsync); + await item.FilterAsync.OnResourceExecutionAsync(_resourceExecutingContext, InvokeResourceFilterAwaitedAsync); _diagnosticSource.AfterOnResourceExecution(_resourceExecutedContext, item.FilterAsync); @@ -294,9 +304,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal } else { + await InvokeResourceFilterAsync(); + Debug.Assert(_resourceExecutedContext != null); + _diagnosticSource.BeforeOnResourceExecuted(_resourceExecutedContext, item.Filter); - item.Filter.OnResourceExecuted(await InvokeResourceFilterAsync()); + item.Filter.OnResourceExecuted(_resourceExecutedContext); _diagnosticSource.AfterOnResourceExecuted(_resourceExecutedContext, item.Filter); } @@ -359,7 +372,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } Debug.Assert(_resourceExecutedContext != null); - return _resourceExecutedContext; } private Task InvokeAllExceptionFiltersAsync() @@ -463,7 +475,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal await InvokeActionFilterAsync(); } - private async Task InvokeActionFilterAsync() + private async Task InvokeActionFilterAwaitedAsync() { Debug.Assert(_actionExecutingContext != null); if (_actionExecutingContext.Result != null) @@ -478,6 +490,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal throw new InvalidOperationException(message); } + await InvokeActionFilterAsync(); + + Debug.Assert(_actionExecutedContext != null); + return _actionExecutedContext; + } + + private async Task InvokeActionFilterAsync() + { + Debug.Assert(_actionExecutingContext != null); + var item = _cursor.GetNextFilter(); try { @@ -485,7 +507,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal { _diagnosticSource.BeforeOnActionExecution(_actionExecutingContext, item.FilterAsync); - await item.FilterAsync.OnActionExecutionAsync(_actionExecutingContext, InvokeActionFilterAsync); + await item.FilterAsync.OnActionExecutionAsync(_actionExecutingContext, InvokeActionFilterAwaitedAsync); _diagnosticSource.AfterOnActionExecution(_actionExecutedContext, item.FilterAsync); @@ -528,9 +550,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal } else { + await InvokeActionFilterAsync(); + Debug.Assert(_actionExecutedContext != null); + _diagnosticSource.BeforeOnActionExecuted(_actionExecutedContext, item.Filter); - item.Filter.OnActionExecuted(await InvokeActionFilterAsync()); + item.Filter.OnActionExecuted(_actionExecutedContext); _diagnosticSource.BeforeOnActionExecuted(_actionExecutedContext, item.Filter); } @@ -578,7 +603,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal ExceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception) }; } - return _actionExecutedContext; + + Debug.Assert(_actionExecutedContext != null); } private async Task InvokeAllResultFiltersAsync(IActionResult result) @@ -603,9 +629,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private async Task InvokeResultFilterAsync() + private async Task InvokeResultFilterAwaitedAsync() { Debug.Assert(_resultExecutingContext != null); + if (_resultExecutingContext.Cancel == true) { // If we get here, it means that an async filter set cancel == true AND called next(). @@ -619,6 +646,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal throw new InvalidOperationException(message); } + await InvokeResultFilterAsync(); + + Debug.Assert(_resultExecutedContext != null); + return _resultExecutedContext; + } + + private async Task InvokeResultFilterAsync() + { + Debug.Assert(_resultExecutingContext != null); + try { var item = _cursor.GetNextFilter(); @@ -626,7 +663,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal { _diagnosticSource.BeforeOnResultExecution(_resultExecutingContext, item.FilterAsync); - await item.FilterAsync.OnResultExecutionAsync(_resultExecutingContext, InvokeResultFilterAsync); + await item.FilterAsync.OnResultExecutionAsync(_resultExecutingContext, InvokeResultFilterAwaitedAsync); _diagnosticSource.AfterOnResultExecution(_resultExecutedContext, item.FilterAsync); @@ -669,9 +706,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal } else { + await InvokeResultFilterAsync(); + Debug.Assert(_resultExecutedContext != null); + _diagnosticSource.BeforeOnResultExecuted(_resultExecutedContext, item.Filter); - item.Filter.OnResultExecuted(await InvokeResultFilterAsync()); + item.Filter.OnResultExecuted(_resultExecutedContext); _diagnosticSource.AfterOnResultExecuted(_resultExecutedContext, item.Filter); } @@ -708,7 +748,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal }; } - return _resultExecutedContext; + Debug.Assert(_resultExecutedContext != null); } private async Task InvokeResultAsync(IActionResult result)