[Fixes #3752] Cleanup Controller invocation pipeline

* Added a Release method to IControllerActivator
* Changed Create in IControllerActivator to take in a ControllerActionContext
* Move the check to determine if a controller can be instantiated into the controller activator.
* Move logic for disposing controllers into the controller activator and make release on the
  controller factory delegate into the activator.
* Changed release methods to take in a controller context.

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs

@@ -2,6 +2,9 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Reflection;
+using Microsoft.AspNetCore.Mvc.Core;
+using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.Mvc.Internal;
 
 namespace Microsoft.AspNetCore.Mvc.Controllers
@@ -19,23 +22,64 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         /// <param name="typeActivatorCache">The <see cref="ITypeActivatorCache"/>.</param>
         public DefaultControllerActivator(ITypeActivatorCache typeActivatorCache)
         {
+            if (typeActivatorCache == null)
+            {
+                throw new ArgumentNullException(nameof(typeActivatorCache));
+            }
+
             _typeActivatorCache = typeActivatorCache;
         }
+
         /// <inheritdoc />
-        public virtual object Create(ActionContext actionContext, Type controllerType)
+        public virtual object Create(ControllerContext controllerContext)
         {
-            if (actionContext == null)
+            if (controllerContext == null)
             {
-                throw new ArgumentNullException(nameof(actionContext));
+                throw new ArgumentNullException(nameof(controllerContext));
             }
 
-            if (controllerType == null)
+            if (controllerContext.ActionDescriptor == null)
             {
-                throw new ArgumentNullException(nameof(controllerType));
+                throw new ArgumentException(Resources.FormatPropertyOfTypeCannotBeNull(
+                    nameof(ControllerContext.ActionDescriptor),
+                    nameof(ControllerContext)));
             }
 
-            var serviceProvider = actionContext.HttpContext.RequestServices;
+            var controllerTypeInfo = controllerContext.ActionDescriptor.ControllerTypeInfo;
-            return _typeActivatorCache.CreateInstance<object>(serviceProvider, controllerType);
+            if (controllerTypeInfo.IsValueType ||
+                controllerTypeInfo.IsInterface ||
+                controllerTypeInfo.IsAbstract ||
+                (controllerTypeInfo.IsGenericType && controllerTypeInfo.IsGenericTypeDefinition))
+            {
+                var message = Resources.FormatValueInterfaceAbstractOrOpenGenericTypesCannotBeActivated(
+                    controllerTypeInfo.FullName,
+                    GetType().FullName);
+
+                throw new InvalidOperationException(message);
+            }
+
+            var serviceProvider = controllerContext.HttpContext.RequestServices;
+            return _typeActivatorCache.CreateInstance<object>(serviceProvider, controllerTypeInfo.AsType());
+        }
+
+        /// <inheritdoc />
+        public virtual void Release(ControllerContext context, object controller)
+        {
+            if (context == null)
+            {
+                throw new ArgumentNullException(nameof(context));
+            }
+
+            if (controller == null)
+            {
+                throw new ArgumentNullException(nameof(controller));
+            }
+
+            var disposable = controller as IDisposable;
+            if (disposable != null)
+            {
+                disposable.Dispose();
+            }
         }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs

@@ -31,6 +31,16 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             IControllerActivator controllerActivator,
             IEnumerable<IControllerPropertyActivator> propertyActivators)
         {
+            if (controllerActivator == null)
+            {
+                throw new ArgumentNullException(nameof(controllerActivator));
+            }
+
+            if (propertyActivators == null)
+            {
+                throw new ArgumentNullException(nameof(propertyActivators));
+            }
+
             _controllerActivator = controllerActivator;
             _propertyActivators = propertyActivators.ToArray();
         }
@@ -61,20 +71,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
                     nameof(ControllerContext)));
             }
 
-            var controllerType = context.ActionDescriptor.ControllerTypeInfo.AsType();
+            var controller = _controllerActivator.Create(context);
-            var controllerTypeInfo = controllerType.GetTypeInfo();
-            if (controllerTypeInfo.IsValueType ||
-                controllerTypeInfo.IsInterface ||
-                controllerTypeInfo.IsAbstract ||
-                (controllerTypeInfo.IsGenericType && controllerTypeInfo.IsGenericTypeDefinition))
-            {
-                var message = Resources.FormatValueInterfaceAbstractOrOpenGenericTypesCannotBeActivated(
-                    controllerType.FullName, 
-                    GetType().FullName);
-                throw new InvalidOperationException(message);
-            }
-
-            var controller = _controllerActivator.Create(context, controllerType);
             foreach (var propertyActivator in _propertyActivators)
             {
                 propertyActivator.Activate(context, controller);
@@ -84,14 +81,19 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         }
 
         /// <inheritdoc />
-        public virtual void ReleaseController(object controller)
+        public virtual void ReleaseController(ControllerContext context, object controller)
         {
-            var disposableController = controller as IDisposable;
+            if (context == null)
-
-            if (disposableController != null)
             {
-                disposableController.Dispose();
+                throw new ArgumentNullException(nameof(context));
             }
+
+            if (controller == null)
+            {
+                throw new ArgumentNullException(nameof(controller));
+            }
+
+            _controllerActivator.Release(context, controller);
         }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs

@@ -13,7 +13,13 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         /// <summary>
         /// Creates a controller.
         /// </summary>
-        /// <param name="context">The <see cref="ActionContext"/> for the executing action.</param>
+        /// <param name="context">The <see cref="ControllerContext"/> for the executing action.</param>
-        object Create(ActionContext context, Type controllerType);
+        object Create(ControllerContext context);
+
+        /// <summary>
+        /// Releases a controller.
+        /// </summary>
+        /// <param name="controller">The controller to release.</param>
+        void Release(ControllerContext context, object controller);
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs

@@ -19,6 +19,6 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         /// Releases a controller instance.
         /// </summary>
         /// <param name="controller">The controller.</param>
-        void ReleaseController(object controller);
+        void ReleaseController(ControllerContext context, object controller);
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs

@@ -13,19 +13,21 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
     public class ServiceBasedControllerActivator : IControllerActivator
     {
         /// <inheritdoc />
-        public object Create(ActionContext actionContext, Type controllerType)
+        public object Create(ControllerContext actionContext)
         {
             if (actionContext == null)
             {
                 throw new ArgumentNullException(nameof(actionContext));
             }
 
-            if (controllerType == null)
+            var controllerType = actionContext.ActionDescriptor.ControllerTypeInfo.AsType();
-            {
-                throw new ArgumentNullException(nameof(controllerType));
-            }
 
             return actionContext.HttpContext.RequestServices.GetRequiredService(controllerType);
         }
+
+        /// <inheritdoc />
+        public virtual void Release(ControllerContext context, object controller)
+        {
+        }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs

@@ -83,11 +83,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal
 
         protected override void ReleaseInstance(object instance)
         {
-            _controllerFactory.ReleaseController(instance);
+            _controllerFactory.ReleaseController(Context, instance);
         }
 
         protected override async Task<IActionResult> InvokeActionAsync(ActionExecutingContext actionExecutingContext)
         {
+            if (actionExecutingContext == null)
+            {
+                throw new ArgumentNullException(nameof(actionExecutingContext));
+            }
+
             var actionMethodInfo = _descriptor.MethodInfo;
             var arguments = ControllerActionExecutor.PrepareArguments(
                 actionExecutingContext.ActionArguments,

test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs

@@ -2,9 +2,13 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Reflection;
 using Microsoft.AspNetCore.Http.Internal;
 using Microsoft.AspNetCore.Mvc.Abstractions;
+using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.Mvc.Internal;
+using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
 using Microsoft.AspNetCore.Routing;
 using Moq;
 using Xunit;
@@ -26,16 +30,79 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             {
                 RequestServices = serviceProvider.Object
             };
-            var actionContext = new ActionContext(httpContext,
+
-                                                  new RouteData(),
+            var context = new ControllerContext(
-                                                  new ActionDescriptor());
+                new ActionContext(
+                    httpContext,
+                    new RouteData(),
+                    new ControllerActionDescriptor
+                    {
+                        ControllerTypeInfo = type.GetTypeInfo()
+                    }));
+
             // Act
-            var instance = activator.Create(actionContext, type);
+            var instance = activator.Create(context);
 
             // Assert
             Assert.IsType(type, instance);
         }
 
+        [Fact]
+        public void Release_DisposesController_IfDisposable()
+        {
+            // Arrange
+            var controller = new MyController();
+            var activator = new DefaultControllerActivator(Mock.Of<ITypeActivatorCache>());
+
+            // Act
+            activator.Release(new ControllerContext(), controller);
+
+            // Assert
+            Assert.Equal(true, controller.Disposed);
+        }
+
+        [Theory]
+        [InlineData(typeof(int))]
+        [InlineData(typeof(OpenGenericType<>))]
+        [InlineData(typeof(AbstractType))]
+        [InlineData(typeof(InterfaceType))]
+        public void CreateController_ThrowsIfControllerCannotBeActivated(Type type)
+        {
+            // Arrange
+            var actionDescriptor = new ControllerActionDescriptor
+            {
+                ControllerTypeInfo = type.GetTypeInfo()
+            };
+
+            var context = new ControllerContext()
+            {
+                ActionDescriptor = actionDescriptor,
+                HttpContext = new DefaultHttpContext()
+                {
+                    RequestServices = GetServices(),
+                },
+            };
+            var factory = new DefaultControllerActivator(new TypeActivatorCache());
+
+            // Act and Assert
+            var exception = Assert.Throws<InvalidOperationException>(() => factory.Create(context));
+            Assert.Equal(
+                $"The type '{type.FullName}' cannot be activated by '{typeof(DefaultControllerActivator).FullName}' " +
+                "because it is either a value type, an interface, an abstract class or an open generic type.",
+                exception.Message);
+        }
+
+        [Fact]
+        public void DefaultControllerActivator_ReleasesNonIDisposableController()
+        {
+            // Arrange
+            var activator = new DefaultControllerActivator(Mock.Of<ITypeActivatorCache>());
+            var controller = new object();
+
+            // Act + Assert (does not throw)
+            activator.Release(Mock.Of<ControllerContext>(), controller);
+        }
+
         [Fact]
         public void Create_TypeActivatesTypesWithServices()
         {
@@ -51,11 +118,18 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             {
                 RequestServices = serviceProvider.Object
             };
-            var actionContext = new ActionContext(httpContext,
+
-                                                  new RouteData(),
+            var context = new ControllerContext(
-                                                  new ActionDescriptor());
+                new ActionContext(
+                    httpContext,
+                    new RouteData(),
+                    new ControllerActionDescriptor
+                    {
+                        ControllerTypeInfo = typeof(TypeDerivingFromControllerWithServices).GetTypeInfo()
+                    }));
+
             // Act
-            var instance = activator.Create(actionContext, typeof(TypeDerivingFromControllerWithServices));
+            var instance = activator.Create(context);
 
             // Assert
             var controller = Assert.IsType<TypeDerivingFromControllerWithServices>(instance);
@@ -81,6 +155,19 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             public TestService TestService { get; }
         }
 
+        private IServiceProvider GetServices()
+        {
+            var metadataProvider = new EmptyModelMetadataProvider();
+            var services = new Mock<IServiceProvider>();
+            services.Setup(s => s.GetService(typeof(IUrlHelper)))
+                .Returns(Mock.Of<IUrlHelper>());
+            services.Setup(s => s.GetService(typeof(IModelMetadataProvider)))
+                .Returns(metadataProvider);
+            services.Setup(s => s.GetService(typeof(IObjectModelValidator)))
+                .Returns(new DefaultObjectValidator(metadataProvider));
+            return services.Object;
+        }
+
         private class PocoType
         {
         }
@@ -88,5 +175,27 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         private class TestService
         {
         }
+
+        private class OpenGenericType<T> : Controller
+        {
+        }
+
+        private abstract class AbstractType : Controller
+        {
+        }
+
+        private interface InterfaceType
+        {
+        }
+
+        private class MyController : IDisposable
+        {
+            public bool Disposed { get; set; }
+
+            public void Dispose()
+            {
+                Disposed = true;
+            }
+        }
     }
 }

test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs

@@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             };
 
             var activator = new Mock<IControllerActivator>();
-            activator.Setup(a => a.Create(context, typeof(MyController)))
+            activator.Setup(a => a.Create(context))
                      .Returns(expected)
                      .Verifiable();
 
@@ -186,61 +186,20 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
                 exception.Message);
         }
 
-        [Theory]
-        [InlineData(typeof(int))]
-        [InlineData(typeof(OpenGenericType<>))]
-        [InlineData(typeof(AbstractType))]
-        [InlineData(typeof(InterfaceType))]
-        public void CreateController_ThrowsIfControllerCannotBeActivated(Type type)
-        {
-            // Arrange
-            var actionDescriptor = new ControllerActionDescriptor
-            {
-                ControllerTypeInfo = type.GetTypeInfo()
-            };
-
-            var context = new ControllerContext()
-            {
-                ActionDescriptor = actionDescriptor,
-                HttpContext = new DefaultHttpContext()
-                {
-                    RequestServices = GetServices(),
-                },
-            };
-            var factory = CreateControllerFactory(new DefaultControllerActivator(new TypeActivatorCache()));
-
-            // Act and Assert
-            var exception = Assert.Throws<InvalidOperationException>(() => factory.CreateController(context));
-            Assert.Equal(
-                $"The type '{type.FullName}' cannot be activated by '{typeof(DefaultControllerFactory).FullName}' " +
-                "because it is either a value type, an interface, an abstract class or an open generic type.",
-                exception.Message);
-        }
-
         [Fact]
-        public void DefaultControllerFactory_DisposesIDisposableController()
+        public void DefaultControllerFactory_DelegatesDisposalToControllerActivator()
         {
             // Arrange
-            var factory = CreateControllerFactory();
+            var activatorMock = new Mock<IControllerActivator>();
+            activatorMock.Setup(s => s.Release(It.IsAny<ControllerContext>(), It.IsAny<object>()));
+
+            var factory = CreateControllerFactory(activatorMock.Object);
             var controller = new MyController();
 
             // Act + Assert
-            Assert.False(controller.Disposed);
+            factory.ReleaseController(new ControllerContext(), controller);
 
-            factory.ReleaseController(controller);
+            activatorMock.Verify();
-
-            Assert.True(controller.Disposed);
-        }
-
-        [Fact]
-        public void DefaultControllerFactory_ReleasesNonIDisposableController()
-        {
-            // Arrange
-            var factory = CreateControllerFactory();
-            var controller = new object();
-
-            // Act + Assert (does not throw)
-            factory.ReleaseController(controller);
         }
 
         private IServiceProvider GetServices()
@@ -258,7 +217,9 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
 
         private static DefaultControllerFactory CreateControllerFactory(IControllerActivator controllerActivator = null)
         {
-            controllerActivator = controllerActivator ?? Mock.Of<IControllerActivator>();
+            var activatorMock = new Mock<IControllerActivator>();
+
+            controllerActivator = controllerActivator ?? activatorMock.Object;
             var propertyActivators = new IControllerPropertyActivator[]
             {
                 new DefaultControllerPropertyActivator(),
@@ -313,21 +274,5 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
         private class TestService
         {
         }
-
-        private class Controller
-        {
-        }
-
-        private class OpenGenericType<T> : Controller
-        {
-        }
-
-        private abstract class AbstractType : Controller
-        {
-        }
-
-        private interface InterfaceType
-        {
-        }
     }
 }

test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/ServiceBasedControllerActivatorTest.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Reflection;
 using Microsoft.AspNetCore.Http.Internal;
 using Microsoft.AspNetCore.Mvc.Abstractions;
 using Microsoft.AspNetCore.Routing;
@@ -26,12 +27,16 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
                 RequestServices = serviceProvider.Object
             };
             var activator = new ServiceBasedControllerActivator();
-            var actionContext = new ActionContext(httpContext,
+            var context = new ControllerContext(new ActionContext(
-                                                  new RouteData(),
+                httpContext,
-                                                  new ActionDescriptor());
+                new RouteData(),
+                new ControllerActionDescriptor
+                {
+                    ControllerTypeInfo = typeof(DIController).GetTypeInfo()
+                }));
 
             // Act
-            var instance = activator.Create(actionContext, typeof(DIController));
+            var instance = activator.Create(context);
 
             // Assert
             Assert.Same(controller, instance);
@@ -44,19 +49,26 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
             // Arrange
             var expected = "No service for type '" + typeof(DIController) + "' has been registered.";
             var controller = new DIController();
-            var serviceProvider = new Mock<IServiceProvider>();
+
             var httpContext = new DefaultHttpContext
             {
-                RequestServices = serviceProvider.Object
+                RequestServices = Mock.Of<IServiceProvider>()
             };
+
             var activator = new ServiceBasedControllerActivator();
-            var actionContext = new ActionContext(httpContext,
+            var context = new ControllerContext(
-                                                  new RouteData(),
+                new ActionContext(
-                                                  new ActionDescriptor());
+                httpContext,
+                new RouteData(),
+                new ControllerActionDescriptor
+                {
+                    ControllerTypeInfo = typeof(DIController).GetTypeInfo()
+                }));
 
             // Act and Assert
             var ex = Assert.Throws<InvalidOperationException>(
-                        () => activator.Create(actionContext, typeof(DIController)));
+                () => activator.Create(context));
+
             Assert.Equal(expected, ex.Message);
         }
 

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs

@@ -2208,7 +2208,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                 return _controller;
             }
 
-            public void ReleaseController(object controller)
+            public void ReleaseController(ControllerContext context, object controller)
             {
                 Assert.NotNull(controller);
                 Assert.Same(_controller, controller);