From 4141fcae6904cffa5b28b89ec098de03c69b6aea Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 31 Dec 2015 16:10:11 -0800 Subject: [PATCH] Optimize MvcRouteHandler - Check MVC services once at startup - Make action selector sync We've never really had a scenario for the action selector being async, it just ended up that way. None of our extensibility here lets you do anything async without replacing it wholesale, which we don't recommend. --- .../Infrastructure/DefaultActionSelector.cs | 17 ++++- .../Infrastructure/IActionSelector.cs | 14 +++- .../Infrastructure/MvcRouteHandler.cs | 34 +++++---- .../DefaultActionSelectorTests.cs | 74 +++++++++---------- .../Infrastructure/MvcRouteHandlerTests.cs | 9 ++- 5 files changed, 83 insertions(+), 65 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs index fb04068fb6..ae692697e0 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/DefaultActionSelector.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Mvc.Abstractions; using Microsoft.AspNet.Mvc.ActionConstraints; @@ -16,12 +15,21 @@ using Microsoft.Extensions.Logging; namespace Microsoft.AspNet.Mvc.Infrastructure { + /// + /// A default implementation. + /// public class DefaultActionSelector : IActionSelector { private readonly IActionSelectorDecisionTreeProvider _decisionTreeProvider; private readonly IActionConstraintProvider[] _actionConstraintProviders; private ILogger _logger; + /// + /// Creates a new . + /// + /// The . + /// The set of instances. + /// The . public DefaultActionSelector( IActionSelectorDecisionTreeProvider decisionTreeProvider, IEnumerable actionConstraintProviders, @@ -32,7 +40,8 @@ namespace Microsoft.AspNet.Mvc.Infrastructure _logger = loggerFactory.CreateLogger(); } - public Task SelectAsync(RouteContext context) + /// + public ActionDescriptor Select(RouteContext context) { if (context == null) { @@ -71,13 +80,13 @@ namespace Microsoft.AspNet.Mvc.Infrastructure if (finalMatches == null || finalMatches.Count == 0) { - return Task.FromResult(null); + return null; } else if (finalMatches.Count == 1) { var selectedAction = finalMatches[0]; - return Task.FromResult(selectedAction); + return selectedAction; } else { diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/IActionSelector.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/IActionSelector.cs index 7fd3d18f0e..80679c79be 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/IActionSelector.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/IActionSelector.cs @@ -1,14 +1,24 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System.Threading.Tasks; using Microsoft.AspNet.Mvc.Abstractions; using Microsoft.AspNet.Routing; namespace Microsoft.AspNet.Mvc.Infrastructure { + /// + /// Defines an interface for selecting an MVC action to invoke for the current request. + /// public interface IActionSelector { - Task SelectAsync(RouteContext context); + /// + /// Selects an for the request associated with . + /// + /// The for the current request. + /// An or null if no action can be selected. + /// + /// Thrown when action selection results in an ambiguity. + /// + ActionDescriptor Select(RouteContext context); } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs index 0b4a44d334..da8686b3e2 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs @@ -38,25 +38,20 @@ namespace Microsoft.AspNet.Mvc.Infrastructure return null; } - public async Task RouteAsync(RouteContext context) + public Task RouteAsync(RouteContext context) { if (context == null) { throw new ArgumentNullException(nameof(context)); } - var services = context.HttpContext.RequestServices; - - // Verify if AddMvc was done before calling UseMvc - // We use the MvcMarkerService to make sure if all the services were added. - MvcServicesHelper.ThrowIfMvcNotRegistered(services); EnsureServices(context.HttpContext); - var actionDescriptor = await _actionSelector.SelectAsync(context); + var actionDescriptor = _actionSelector.Select(context); if (actionDescriptor == null) { _logger.NoActionsMatched(); - return; + return TaskCache.CompletedTask; } if (actionDescriptor.RouteValueDefaults != null) @@ -68,12 +63,13 @@ namespace Microsoft.AspNet.Mvc.Infrastructure context.RouteData.Values.Add(kvp.Key, kvp.Value); } } + + // Removing RouteGroup from RouteValues to simulate the result of conventional routing + context.RouteData.Values.Remove(TreeRouter.RouteGroupKey); } - // Removing RouteGroup from RouteValues to simulate the result of conventional routing - context.RouteData.Values.Remove(TreeRouter.RouteGroupKey); - context.Handler = (c) => InvokeActionAsync(c, actionDescriptor); + return TaskCache.CompletedTask; } private async Task InvokeActionAsync(HttpContext httpContext, ActionDescriptor actionDescriptor) @@ -121,15 +117,21 @@ namespace Microsoft.AspNet.Mvc.Infrastructure return; } + var services = context.RequestServices; + + // Verify if AddMvc was done before calling UseMvc + // We use the MvcMarkerService to make sure if all the services were added. + MvcServicesHelper.ThrowIfMvcNotRegistered(services); + // The IActionContextAccessor is optional. We want to avoid the overhead of using CallContext // if possible. - _actionContextAccessor = context.RequestServices.GetService(); + _actionContextAccessor = services.GetService(); - _actionInvokerFactory = context.RequestServices.GetRequiredService(); - _actionSelector = context.RequestServices.GetRequiredService(); - _diagnosticSource = context.RequestServices.GetRequiredService(); + _actionInvokerFactory = services.GetRequiredService(); + _actionSelector = services.GetRequiredService(); + _diagnosticSource = services.GetRequiredService(); - var factory = context.RequestServices.GetRequiredService(); + var factory = services.GetRequiredService(); _logger = factory.CreateLogger(); _servicesRetrieved = true; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/DefaultActionSelectorTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/DefaultActionSelectorTests.cs index a4221d16c4..0e63851ffe 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/DefaultActionSelectorTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/DefaultActionSelectorTests.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; using System.Linq; using System.Reflection; -using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Http.Internal; using Microsoft.AspNet.Mvc.Abstractions; @@ -25,7 +24,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure public class DefaultActionSelectorTests { [Fact] - public async void SelectAsync_AmbiguousActions_LogIsCorrect() + public void Select_AmbiguousActions_LogIsCorrect() { // Arrange var sink = new TestSink(); @@ -44,10 +43,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure $"ambiguity. Matching actions: {actionNames}"; // Act - await Assert.ThrowsAsync(async () => - { - await selector.SelectAsync(routeContext); - }); + Assert.Throws(() => { selector.Select(routeContext); }); // Assert Assert.Empty(sink.Scopes); @@ -56,7 +52,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure } [Fact] - public async Task SelectAsync_PrefersActionWithConstraints() + public void Select_PrefersActionWithConstraints() { // Arrange var actionWithConstraints = new ActionDescriptor() @@ -79,14 +75,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, actionWithConstraints); } [Fact] - public async Task SelectAsync_ConstraintsRejectAll() + public void Select_ConstraintsRejectAll() { // Arrange var action1 = new ActionDescriptor() @@ -111,14 +107,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Null(action); } [Fact] - public async Task SelectAsync_ConstraintsRejectAll_DifferentStages() + public void Select_ConstraintsRejectAll_DifferentStages() { // Arrange var action1 = new ActionDescriptor() @@ -145,14 +141,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Null(action); } [Fact] - public async Task SelectAsync_ActionConstraintFactory() + public void Select_ActionConstraintFactory() { // Arrange var actionWithConstraints = new ActionDescriptor() @@ -177,14 +173,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, actionWithConstraints); } [Fact] - public async Task SelectAsync_ActionConstraintFactory_ReturnsNull() + public void Select_ActionConstraintFactory_ReturnsNull() { // Arrange var nullConstraint = new ActionDescriptor() @@ -203,7 +199,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, nullConstraint); @@ -211,7 +207,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure // There's a custom constraint provider registered that only understands BooleanConstraintMarker [Fact] - public async Task SelectAsync_CustomProvider() + public void Select_CustomProvider() { // Arrange var actionWithConstraints = new ActionDescriptor() @@ -233,7 +229,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, actionWithConstraints); @@ -241,7 +237,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure // Due to ordering of stages, the first action will be better. [Fact] - public async Task SelectAsync_ConstraintsInOrder() + public void Select_ConstraintsInOrder() { // Arrange var best = new ActionDescriptor() @@ -266,7 +262,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, best); @@ -274,7 +270,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure // Due to ordering of stages, the first action will be better. [Fact] - public async Task SelectAsync_ConstraintsInOrder_MultipleStages() + public void Select_ConstraintsInOrder_MultipleStages() { // Arrange var best = new ActionDescriptor() @@ -303,14 +299,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, best); } [Fact] - public async Task SelectAsync_Fallback_ToActionWithoutConstraints() + public void Select_Fallback_ToActionWithoutConstraints() { // Arrange var nomatch1 = new ActionDescriptor() @@ -341,14 +337,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext("POST"); // Act - var action = await selector.SelectAsync(context); + var action = selector.Select(context); // Assert Assert.Same(action, best); } [Fact] - public async Task SelectAsync_Ambiguous() + public void Select_Ambiguous() { // Arrange var expectedMessage = @@ -375,9 +371,9 @@ namespace Microsoft.AspNet.Mvc.Infrastructure context.RouteData.Values.Add("action", "Buy"); // Act - var ex = await Assert.ThrowsAsync(async () => + var ex = Assert.Throws(() => { - await selector.SelectAsync(context); + selector.Select(context); }); // Assert @@ -390,7 +386,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure [InlineData("POST")] [InlineData("DELETE")] [InlineData("PATCH")] - public async Task HttpMethodAttribute_ActionWithMultipleHttpMethodAttributeViaAcceptVerbs_ORsMultipleHttpMethods(string verb) + public void HttpMethodAttribute_ActionWithMultipleHttpMethodAttributeViaAcceptVerbs_ORsMultipleHttpMethods(string verb) { // Arrange var routeContext = new RouteContext(GetHttpContext(verb)); @@ -398,7 +394,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure routeContext.RouteData.Values.Add("action", "Patch"); // Act - var result = await InvokeActionSelector(routeContext); + var result = InvokeActionSelector(routeContext); // Assert Assert.Equal("Patch", result.Name); @@ -411,7 +407,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure [InlineData("DELETE")] [InlineData("PATCH")] [InlineData("HEAD")] - public async Task HttpMethodAttribute_ActionWithMultipleHttpMethodAttributes_ORsMultipleHttpMethods(string verb) + public void HttpMethodAttribute_ActionWithMultipleHttpMethodAttributes_ORsMultipleHttpMethods(string verb) { // Arrange var routeContext = new RouteContext(GetHttpContext(verb)); @@ -419,7 +415,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure routeContext.RouteData.Values.Add("action", "Put"); // Act - var result = await InvokeActionSelector(routeContext); + var result = InvokeActionSelector(routeContext); // Assert Assert.Equal("Put", result.Name); @@ -428,7 +424,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure [Theory] [InlineData("GET")] [InlineData("PUT")] - public async Task HttpMethodAttribute_ActionDecoratedWithHttpMethodAttribute_OverridesConvention(string verb) + public void HttpMethodAttribute_ActionDecoratedWithHttpMethodAttribute_OverridesConvention(string verb) { // Arrange // Note no action name is passed, hence should return a null action descriptor. @@ -436,7 +432,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure routeContext.RouteData.Values.Add("controller", "HttpMethodAttributeTests_RestOnly"); // Act - var result = await InvokeActionSelector(routeContext); + var result = InvokeActionSelector(routeContext); // Assert Assert.Null(result); @@ -466,7 +462,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure [InlineData("POST")] [InlineData("DELETE")] [InlineData("PATCH")] - public async Task ActionNameAttribute_ActionGetsExposedViaActionName_UnreachableByConvention(string verb) + public void ActionNameAttribute_ActionGetsExposedViaActionName_UnreachableByConvention(string verb) { // Arrange var routeContext = new RouteContext(GetHttpContext(verb)); @@ -474,7 +470,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure routeContext.RouteData.Values.Add("action", "RPCMethodWithHttpGet"); // Act - var result = await InvokeActionSelector(routeContext); + var result = InvokeActionSelector(routeContext); // Assert Assert.Null(result); @@ -496,7 +492,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure [InlineData("POST", "CustomActionName_RpcMethod")] [InlineData("DELETE", "CustomActionName_RpcMethod")] [InlineData("PATCH", "CustomActionName_RpcMethod")] - public async Task ActionNameAttribute_DifferentActionName_UsesActionNameFromActionNameAttribute(string verb, string actionName) + public void ActionNameAttribute_DifferentActionName_UsesActionNameFromActionNameAttribute(string verb, string actionName) { // Arrange var routeContext = new RouteContext(GetHttpContext(verb)); @@ -504,13 +500,13 @@ namespace Microsoft.AspNet.Mvc.Infrastructure routeContext.RouteData.Values.Add("action", actionName); // Act - var result = await InvokeActionSelector(routeContext); + var result = InvokeActionSelector(routeContext); // Assert Assert.Equal(actionName, result.Name); } - private async Task InvokeActionSelector(RouteContext context) + private ActionDescriptor InvokeActionSelector(RouteContext context) { var actionDescriptorProvider = GetActionDescriptorProvider(); @@ -536,7 +532,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure actionConstraintProviders, NullLoggerFactory.Instance); - return await defaultActionSelector.SelectAsync(context); + return defaultActionSelector.Select(context); } private ControllerActionDescriptorProvider GetActionDescriptorProvider() diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs index 45fd619168..ec6665eeb6 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs @@ -58,8 +58,9 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var loggerFactory = new TestLoggerFactory(sink, enabled: true); var mockActionSelector = new Mock(); - mockActionSelector.Setup(a => a.SelectAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + mockActionSelector + .Setup(a => a.Select(It.IsAny())) + .Returns(null); var context = CreateRouteContext( actionSelector: mockActionSelector.Object, @@ -172,8 +173,8 @@ namespace Microsoft.AspNet.Mvc.Infrastructure if (actionSelector == null) { var mockActionSelector = new Mock(); - mockActionSelector.Setup(a => a.SelectAsync(It.IsAny())) - .Returns(Task.FromResult(actionDescriptor)); + mockActionSelector.Setup(a => a.Select(It.IsAny())) + .Returns(actionDescriptor); actionSelector = mockActionSelector.Object; }