Changing when controllers are created

This change moves controller creation to the stage immediately before
model binding. The controller will be disposed/released before Resource
Filters run their 'OnResourceExecuted' method. Previously the controller's
lifetime surrounded all filter invocation.

Additionally, the Controller property is now gone from ActionContext, and
is moved to the 4 filter contexts that should have access to it
Action*Context and Result*Context.

Author: rynowak

src/Microsoft.AspNet.Mvc.Core/ActionContext.cs

@@ -14,7 +14,6 @@ public ActionContext([NotNull] ActionContext actionContext)
             : this(actionContext.HttpContext, actionContext.RouteData, actionContext.ActionDescriptor)
         {
             ModelState = actionContext.ModelState;
-            Controller = actionContext.Controller;
         }
 
         public ActionContext([NotNull] RouteContext routeContext, [NotNull] ActionDescriptor actionDescriptor)
@@ -39,10 +38,5 @@ public ActionContext([NotNull] HttpContext httpContext,
         public ModelStateDictionary ModelState { get; private set; }
 
         public ActionDescriptor ActionDescriptor { get; private set; }
-
-        /// <summary>
-        /// The controller is available only after the controller factory runs.
-        /// </summary>
-        public object Controller { get; set; }
     }
 }

src/Microsoft.AspNet.Mvc.Core/ControllerActionInvoker.cs

@@ -50,29 +50,24 @@ public ControllerActionInvoker(
             }
         }
 
-        public async override Task InvokeAsync()
+        protected override object CreateInstance()
         {
             // The binding context is used in activation
             Debug.Assert(ActionBindingContext != null);
-            var controller = _controllerFactory.CreateController(ActionContext);
+            return _controllerFactory.CreateController(ActionContext);
+        }
 
-            try
-            {
-                ActionContext.Controller = controller;
-                await base.InvokeAsync();
-            }
-            finally
-            {
-                _controllerFactory.ReleaseController(ActionContext.Controller);
-            }
+        protected override void ReleaseInstance(object instance)
+        {
+            _controllerFactory.ReleaseController(instance);
         }
 
         protected override async Task<IActionResult> InvokeActionAsync(ActionExecutingContext actionExecutingContext)
         {
             var actionMethodInfo = _descriptor.MethodInfo;
             var actionReturnValue = await ControllerActionExecutor.ExecuteAsync(
                 actionMethodInfo,
-                ActionContext.Controller,
+                actionExecutingContext.Controller,
                 actionExecutingContext.ActionArguments);
 
             var actionResult = CreateActionResult(

src/Microsoft.AspNet.Mvc.Core/DefaultControllerFactory.cs

@@ -37,7 +37,6 @@ public object CreateController(ActionContext actionContext)
                 _serviceProvider,
                 actionDescriptor.ControllerTypeInfo.AsType());
 
-            actionContext.Controller = controller;
             _controllerActivator.Activate(controller, actionContext);
 
             return controller;

src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs

@@ -55,8 +55,6 @@ public FilterActionInvoker(
             _modelValidatorProviderProvider = modelValidatorProviderProvider;
             _valueProviderFactoryProvider = valueProviderFactoryProvider;
             _actionBindingContextAccessor = actionBindingContextAccessor;
-
-            ActionBindingContext = new ActionBindingContext();
         }
 
         protected ActionContext ActionContext { get; private set; }
@@ -73,6 +71,21 @@ private set
             }
         }
 
+        protected object Instance { get; private set; }
+
+        /// <summary>
+        /// Called to create an instance of an object which will act as the reciever of the action invocation.
+        /// </summary>
+        /// <returns>The constructed instance or <c>null</c>.</returns>
+        protected abstract object CreateInstance();
+
+        /// <summary>
+        /// Called to create an instance of an object which will act as the reciever of the action invocation.
+        /// </summary>
+        /// <param name="instance">The instance to release.</param>
+        /// <remarks>This method will not be called if <see cref="CreateInstance"/> returns <c>null</c>.</remarks>
+        protected abstract void ReleaseInstance(object instance);
+
         protected abstract Task<IActionResult> InvokeActionAsync(ActionExecutingContext actionExecutingContext);
 
         protected abstract Task<IDictionary<string, object>> GetActionArgumentsAsync(
@@ -95,7 +108,20 @@ public virtual async Task InvokeAsync()
                 return;
             }
 
-            await InvokeAllResourceFiltersAsync();
+            try
+            {
+                await InvokeAllResourceFiltersAsync();
+            }
+            finally
+            {
+                // Release the instance after all filters have run. We don't need to surround
+                // Authorizations filters because the instance will be created much later than
+                // that.
+                if (Instance != null)
+                {
+                    ReleaseInstance(Instance);
+                }
+            }
 
             // We've reached the end of resource filters. If there's an unhandled exception on the context then
             // it should be thrown and middleware has a chance to handle it.
@@ -207,7 +233,7 @@ private async Task<ResourceExecutedContext> InvokeResourceFilterAsync()
                 if (item.FilterAsync != null)
                 {
                     await item.FilterAsync.OnResourceExecutionAsync(
-                        _resourceExecutingContext, 
+                        _resourceExecutingContext,
                         InvokeResourceFilterAsync);
 
                     if (_resourceExecutedContext == null)
@@ -384,23 +410,30 @@ private async Task InvokeAllActionFiltersAsync()
 
             Debug.Assert(_resourceExecutingContext != null);
 
-            Debug.Assert(ActionBindingContext != null);
+            ActionBindingContext = new ActionBindingContext();
             ActionBindingContext.InputFormatters = _resourceExecutingContext.InputFormatters;
             ActionBindingContext.ModelBinder = new CompositeModelBinder(_resourceExecutingContext.ModelBinders);
             ActionBindingContext.ValidatorProvider = new CompositeModelValidatorProvider(
                 _resourceExecutingContext.ValidatorProviders);
 
             var valueProviderFactoryContext = new ValueProviderFactoryContext(
-                ActionContext.HttpContext, 
+                ActionContext.HttpContext,
                 ActionContext.RouteData.Values);
 
             ActionBindingContext.ValueProvider = CompositeValueProvider.Create(
                 _resourceExecutingContext.ValueProviderFactories,
                 valueProviderFactoryContext);
 
+            Instance = CreateInstance();
+
             var arguments = await GetActionArgumentsAsync(ActionContext, ActionBindingContext);
 
-            _actionExecutingContext = new ActionExecutingContext(ActionContext, _filters, arguments);
+            _actionExecutingContext = new ActionExecutingContext(
+                ActionContext,
+                _filters,
+                arguments,
+                Instance);
+
             await InvokeActionFilterAsync();
         }
 
@@ -429,7 +462,10 @@ private async Task<ActionExecutedContext> InvokeActionFilterAsync()
                     if (_actionExecutedContext == null)
                     {
                         // If we get here then the filter didn't call 'next' indicating a short circuit
-                        _actionExecutedContext = new ActionExecutedContext(_actionExecutingContext, _filters)
+                        _actionExecutedContext = new ActionExecutedContext(
+                            _actionExecutingContext,
+                            _filters,
+                            Instance)
                         {
                             Canceled = true,
                             Result = _actionExecutingContext.Result,
@@ -443,7 +479,10 @@ private async Task<ActionExecutedContext> InvokeActionFilterAsync()
                     if (_actionExecutingContext.Result != null)
                     {
                         // Short-circuited by setting a result.
-                        _actionExecutedContext = new ActionExecutedContext(_actionExecutingContext, _filters)
+                        _actionExecutedContext = new ActionExecutedContext(
+                            _actionExecutingContext,
+                            _filters,
+                            Instance)
                         {
                             Canceled = true,
                             Result = _actionExecutingContext.Result,
@@ -457,7 +496,10 @@ private async Task<ActionExecutedContext> InvokeActionFilterAsync()
                 else
                 {
                     // All action filters have run, execute the action method.
-                    _actionExecutedContext = new ActionExecutedContext(_actionExecutingContext, _filters)
+                    _actionExecutedContext = new ActionExecutedContext(
+                        _actionExecutingContext,
+                        _filters,
+                        Instance)
                     {
                         Result = await InvokeActionAsync(_actionExecutingContext),
                     };
@@ -466,7 +508,10 @@ private async Task<ActionExecutedContext> InvokeActionFilterAsync()
             catch (Exception exception)
             {
                 // Exceptions thrown by the action method OR filters bubble back up through ActionExcecutedContext.
-                _actionExecutedContext = new ActionExecutedContext(_actionExecutingContext, _filters)
+                _actionExecutedContext = new ActionExecutedContext(
+                    _actionExecutingContext,
+                    _filters,
+                    Instance)
                 {
                     ExceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception)
                 };
@@ -478,7 +523,7 @@ private async Task InvokeAllResultFiltersAsync(IActionResult result)
         {
             _cursor.SetStage(FilterStage.ResultFilters);
 
-            _resultExecutingContext = new ResultExecutingContext(ActionContext, _filters, result);
+            _resultExecutingContext = new ResultExecutingContext(ActionContext, _filters, result, Instance);
             await InvokeResultFilterAsync();
 
             Debug.Assert(_resultExecutingContext != null);
@@ -525,7 +570,8 @@ private async Task<ResultExecutedContext> InvokeResultFilterAsync()
                         _resultExecutedContext = new ResultExecutedContext(
                             _resultExecutingContext,
                             _filters,
-                            _resultExecutingContext.Result)
+                            _resultExecutingContext.Result,
+                            Instance)
                         {
                             Canceled = true,
                         };
@@ -536,7 +582,8 @@ private async Task<ResultExecutedContext> InvokeResultFilterAsync()
                         _resultExecutedContext = new ResultExecutedContext(
                             _resultExecutingContext,
                             _filters,
-                            _resultExecutingContext.Result)
+                            _resultExecutingContext.Result,
+                            Instance)
                         {
                             Canceled = true,
                         };
@@ -552,7 +599,8 @@ private async Task<ResultExecutedContext> InvokeResultFilterAsync()
                         _resultExecutedContext = new ResultExecutedContext(
                             _resultExecutingContext,
                             _filters,
-                            _resultExecutingContext.Result)
+                            _resultExecutingContext.Result,
+                            Instance)
                         {
                             Canceled = true,
                         };
@@ -570,15 +618,17 @@ private async Task<ResultExecutedContext> InvokeResultFilterAsync()
                     _resultExecutedContext = new ResultExecutedContext(
                         _resultExecutingContext,
                         _filters,
-                        _resultExecutingContext.Result);
+                        _resultExecutingContext.Result,
+                        Instance);
                 }
             }
             catch (Exception exception)
             {
                 _resultExecutedContext = new ResultExecutedContext(
                     _resultExecutingContext,
                     _filters,
-                    _resultExecutingContext.Result)
+                    _resultExecutingContext.Result,
+                    Instance)
                 {
                     ExceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception)
                 };

src/Microsoft.AspNet.Mvc.Core/Filters/ActionExecutedContext.cs

@@ -14,13 +14,17 @@ public class ActionExecutedContext : FilterContext
 
         public ActionExecutedContext(
             [NotNull] ActionContext actionContext,
-            [NotNull] IList<IFilter> filters)
+            [NotNull] IList<IFilter> filters,
+            object controller)
             : base(actionContext, filters)
         {
+            Controller = controller;
         }
 
         public virtual bool Canceled { get; set; }
 
+        public virtual object Controller { get; }
+
         public virtual Exception Exception
         {
             get

src/Microsoft.AspNet.Mvc.Core/Filters/ActionExecutingContext.cs

@@ -10,14 +10,18 @@ public class ActionExecutingContext : FilterContext
         public ActionExecutingContext(
             [NotNull] ActionContext actionContext,
             [NotNull] IList<IFilter> filters,
-            [NotNull] IDictionary<string, object> actionArguments)
+            [NotNull] IDictionary<string, object> actionArguments,
+            object controller)
             : base(actionContext, filters)
         {
             ActionArguments = actionArguments;
+            Controller = controller;
         }
 
         public virtual IActionResult Result { get; set; }
 
-        public virtual IDictionary<string, object> ActionArguments { get; private set; }
+        public virtual IDictionary<string, object> ActionArguments { get; }
+
+        public virtual object Controller { get; }
     }
 }

src/Microsoft.AspNet.Mvc.Core/Filters/ResultExecutedContext.cs

@@ -15,14 +15,18 @@ public class ResultExecutedContext : FilterContext
         public ResultExecutedContext(
             [NotNull] ActionContext actionContext,
             [NotNull] IList<IFilter> filters,
-            [NotNull] IActionResult result)
+            [NotNull] IActionResult result,
+            object controller)
             : base(actionContext, filters)
         {
             Result = result;
+            Controller = controller;
         }
 
         public virtual bool Canceled { get; set; }
 
+        public virtual object Controller { get; }
+
         public virtual Exception Exception
         {
             get

src/Microsoft.AspNet.Mvc.Core/Filters/ResultExecutingContext.cs

@@ -10,12 +10,16 @@ public class ResultExecutingContext : FilterContext
         public ResultExecutingContext(
             [NotNull] ActionContext actionContext,
             [NotNull] IList<IFilter> filters,
-            [NotNull] IActionResult result)
+            [NotNull] IActionResult result,
+            object controller)
             : base(actionContext, filters)
         {
             Result = result;
+            Controller = controller;
         }
 
+        public virtual object Controller { get; }
+
         public virtual IActionResult Result { get; set; }
 
         public virtual bool Cancel { get; set; }