From 6d78f8adb30c50db7c28867f2507a4b15f504494 Mon Sep 17 00:00:00 2001 From: Yishai Galatzer Date: Thu, 5 Jun 2014 18:30:36 -0700 Subject: [PATCH] Cache action descriptor providers and provide a race safe data structure to get the version. The default implementation has a safe race, and does not allow for action description addition at runtime. It can be replaced with an implementation that can reload. Consumers of the new service that do extra caching are now responsible to look at the version and change the implementation. --- .../ActionDescriptorsCollection.cs | 34 ++++++++++++ ...aultActionDescriptorsCollectionProvider.cs | 53 +++++++++++++++++++ .../DefaultActionSelector.cs | 31 +++++++---- .../IActionDescriptorsCollectionProvider.cs | 25 +++++++++ .../Microsoft.AspNet.Mvc.Core.kproj | 3 ++ .../Microsoft.AspNet.Mvc.kproj | 2 +- src/Microsoft.AspNet.Mvc/MvcServices.cs | 1 + .../ActionAttributeTests.cs | 16 +++++- .../ActionSelectionConventionTests.cs | 18 +++++-- .../DefaultActionSelectorTest.cs | 8 +-- .../BasicTests.cs | 26 +++++++++ .../ActionDescriptorCreationCounter.cs | 41 ++++++++++++++ test/WebSites/BasicWebSite/BasicWebSite.kproj | 1 + .../Controllers/HomeController.cs | 2 +- .../Controllers/MonitorController.cs | 21 ++++++++ test/WebSites/BasicWebSite/Startup.cs | 5 +- 16 files changed, 266 insertions(+), 21 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/ActionDescriptorsCollection.cs create mode 100644 src/Microsoft.AspNet.Mvc.Core/DefaultActionDescriptorsCollectionProvider.cs create mode 100644 src/Microsoft.AspNet.Mvc.Core/IActionDescriptorsCollectionProvider.cs create mode 100644 test/WebSites/BasicWebSite/ActionDescriptorCreationCounter.cs create mode 100644 test/WebSites/BasicWebSite/Controllers/MonitorController.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/ActionDescriptorsCollection.cs b/src/Microsoft.AspNet.Mvc.Core/ActionDescriptorsCollection.cs new file mode 100644 index 0000000000..25e472c0e6 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/ActionDescriptorsCollection.cs @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; + +namespace Microsoft.AspNet.Mvc +{ + /// + /// A cached collection of . + /// + public class ActionDescriptorsCollection + { + /// + /// Initializes a new instance of the . + /// + /// The result of action discovery + /// The unique version of discovered actions. + public ActionDescriptorsCollection([NotNull] IReadOnlyList items, int version) + { + Items = items; + Version = version; + } + + /// + /// Returns the cached . + /// + public IReadOnlyList Items { get; private set; } + + /// + /// Returns the unique version of the currently cached items. + /// + public int Version { get; private set; } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultActionDescriptorsCollectionProvider.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultActionDescriptorsCollectionProvider.cs new file mode 100644 index 0000000000..dc8b68ffdd --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/DefaultActionDescriptorsCollectionProvider.cs @@ -0,0 +1,53 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Microsoft.Framework.DependencyInjection; + +namespace Microsoft.AspNet.Mvc +{ + /// + /// Default implementation for ActionDescriptors. + /// This implementation caches the results at first call, and is not responsible for updates. + /// + public class DefaultActionDescriptorsCollectionProvider : IActionDescriptorsCollectionProvider + { + private readonly IServiceProvider _serviceProvider; + private ActionDescriptorsCollection _collection; + + /// + /// Initializes a new instance of the class. + /// + /// The application IServiceProvider. + public DefaultActionDescriptorsCollectionProvider(IServiceProvider serviceProvider) + { + _serviceProvider = serviceProvider; + } + + /// + /// Returns a cached collection of . + /// + public ActionDescriptorsCollection ActionDescriptors + { + get + { + if (_collection == null) + { + _collection = GetCollection(); + } + + return _collection; + } + } + + private ActionDescriptorsCollection GetCollection() + { + var actionDescriptorProvider = _serviceProvider.GetService>(); + var actionDescriptorProviderContext = new ActionDescriptorProviderContext(); + + actionDescriptorProvider.Invoke(actionDescriptorProviderContext); + + return new ActionDescriptorsCollection(actionDescriptorProviderContext.Results, 0); + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultActionSelector.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultActionSelector.cs index 5e34ef8c54..90682df27e 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DefaultActionSelector.cs +++ b/src/Microsoft.AspNet.Mvc.Core/DefaultActionSelector.cs @@ -9,19 +9,18 @@ using System.Threading.Tasks; using Microsoft.AspNet.Mvc.Core; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Routing; -using Microsoft.Framework.DependencyInjection; namespace Microsoft.AspNet.Mvc { public class DefaultActionSelector : IActionSelector { - private readonly INestedProviderManager _actionDescriptorProvider; + private readonly IActionDescriptorsCollectionProvider _actionDescriptorsCollectionProvider; private readonly IActionBindingContextProvider _bindingProvider; - public DefaultActionSelector(INestedProviderManager actionDescriptorProvider, + public DefaultActionSelector(IActionDescriptorsCollectionProvider actionDescriptorsCollectionProvider, IActionBindingContextProvider bindingProvider) { - _actionDescriptorProvider = actionDescriptorProvider; + _actionDescriptorsCollectionProvider = actionDescriptorsCollectionProvider; _bindingProvider = bindingProvider; } @@ -164,7 +163,7 @@ namespace Microsoft.AspNet.Mvc // Read further for details. public virtual IEnumerable GetCandidateActions(VirtualPathContext context) { - // This method attemptss to find a unique 'best' candidate set of actions from the provided route + // This method attempts to find a unique 'best' candidate set of actions from the provided route // values and ambient route values. // // The purpose of this process is to avoid allowing certain routes to be too greedy. When a route uses @@ -332,12 +331,26 @@ namespace Microsoft.AspNet.Mvc return exemplar; } - private List GetActions() + private IReadOnlyList GetActions() { - var actionDescriptorProviderContext = new ActionDescriptorProviderContext(); - _actionDescriptorProvider.Invoke(actionDescriptorProviderContext); + var descriptors = _actionDescriptorsCollectionProvider.ActionDescriptors; - return actionDescriptorProviderContext.Results; + if (descriptors == null) + { + throw new InvalidOperationException( + Resources.FormatPropertyOfTypeCannotBeNull(_actionDescriptorsCollectionProvider.GetType(), + "ActionDescriptors")); + } + + var items = descriptors.Items; + + if (items == null) + { + throw new InvalidOperationException( + Resources.FormatPropertyOfTypeCannotBeNull(descriptors.GetType(), "Items")); + } + + return items; } private class ActionDescriptorCandidate diff --git a/src/Microsoft.AspNet.Mvc.Core/IActionDescriptorsCollectionProvider.cs b/src/Microsoft.AspNet.Mvc.Core/IActionDescriptorsCollectionProvider.cs new file mode 100644 index 0000000000..7e3917c756 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/IActionDescriptorsCollectionProvider.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNet.Mvc +{ + /// + /// Provides the currently cached collection of . + /// + /// + /// The default implementation, does not update the cache, it is up to the user + /// to create or use an implementation that can update the available actions in + /// the application. The implementor is also responsible for updating the + /// in a thread safe way. + /// + /// Default consumers of this service, are aware of the version and will recache + /// data as appropriate, but rely on the version being unique. + /// + public interface IActionDescriptorsCollectionProvider + { + /// + /// Returns the current cached + /// + ActionDescriptorsCollection ActionDescriptors { get; } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj index e2a77b035b..c412c9ad65 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj +++ b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj @@ -26,6 +26,7 @@ + @@ -67,6 +68,7 @@ + @@ -122,6 +124,7 @@ + diff --git a/src/Microsoft.AspNet.Mvc/Microsoft.AspNet.Mvc.kproj b/src/Microsoft.AspNet.Mvc/Microsoft.AspNet.Mvc.kproj index b9bc5ac0ea..aed005034f 100644 --- a/src/Microsoft.AspNet.Mvc/Microsoft.AspNet.Mvc.kproj +++ b/src/Microsoft.AspNet.Mvc/Microsoft.AspNet.Mvc.kproj @@ -25,4 +25,4 @@ - + \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc/MvcServices.cs b/src/Microsoft.AspNet.Mvc/MvcServices.cs index 27784799cb..6147867aa4 100644 --- a/src/Microsoft.AspNet.Mvc/MvcServices.cs +++ b/src/Microsoft.AspNet.Mvc/MvcServices.cs @@ -48,6 +48,7 @@ namespace Microsoft.AspNet.Mvc ReflectedRouteConstraintsActionDescriptorProvider>(); yield return describe.Transient, ReflectedActionInvokerProvider>(); + yield return describe.Singleton(); yield return describe.Transient(); yield return describe.Transient(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ActionAttributeTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ActionAttributeTests.cs index 41de226cae..29c62a2316 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ActionAttributeTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ActionAttributeTests.cs @@ -5,10 +5,12 @@ using System; using System.Collections.Generic; +using System.ComponentModel.Design; using System.Linq; using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNet.Http; +using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.DependencyInjection.NestedProviders; using Moq; using Xunit; @@ -190,14 +192,24 @@ namespace Microsoft.AspNet.Mvc.Core.Test return await InvokeActionSelector(context, _actionDiscoveryConventions); } - private async Task InvokeActionSelector(RouteContext context, DefaultActionDiscoveryConventions actionDiscoveryConventions) + private async Task InvokeActionSelector(RouteContext context, + DefaultActionDiscoveryConventions actionDiscoveryConventions) { var actionDescriptorProvider = GetActionDescriptorProvider(actionDiscoveryConventions); var descriptorProvider = new NestedProviderManager(new[] { actionDescriptorProvider }); + + var serviceContainer = new ServiceContainer(); + serviceContainer.AddService(typeof(INestedProviderManager), + descriptorProvider); + + var actionCollectionDescriptorProvider = new DefaultActionDescriptorsCollectionProvider(serviceContainer); + var bindingProvider = new Mock(); - var defaultActionSelector = new DefaultActionSelector(descriptorProvider, bindingProvider.Object); + var defaultActionSelector = new DefaultActionSelector(actionCollectionDescriptorProvider, + bindingProvider.Object); + return await defaultActionSelector.SelectAsync(context); } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ActionSelectionConventionTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ActionSelectionConventionTests.cs index c276d16125..9345bc1be2 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ActionSelectionConventionTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ActionSelectionConventionTests.cs @@ -5,10 +5,12 @@ using System; using System.Collections.Generic; +using System.ComponentModel.Design; using System.Reflection; using System.Threading.Tasks; -using Microsoft.AspNet.Http; using Microsoft.AspNet.Routing; +using Microsoft.AspNet.Http; +using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.DependencyInjection.NestedProviders; using Moq; using Xunit; @@ -158,14 +160,24 @@ namespace Microsoft.AspNet.Mvc.Core.Test return await InvokeActionSelector(context, _actionDiscoveryConventions); } - private async Task InvokeActionSelector(RouteContext context, DefaultActionDiscoveryConventions actionDiscoveryConventions) + private async Task InvokeActionSelector(RouteContext context, + DefaultActionDiscoveryConventions actionDiscoveryConventions) { var actionDescriptorProvider = GetActionDescriptorProvider(actionDiscoveryConventions); var descriptorProvider = new NestedProviderManager(new[] { actionDescriptorProvider }); + + var serviceContainer = new ServiceContainer(); + serviceContainer.AddService(typeof(INestedProviderManager), + descriptorProvider); + + var actionCollectionDescriptorProvider = new DefaultActionDescriptorsCollectionProvider(serviceContainer); + var bindingProvider = new Mock(); - var defaultActionSelector = new DefaultActionSelector(descriptorProvider, bindingProvider.Object); + var defaultActionSelector = new DefaultActionSelector(actionCollectionDescriptorProvider, + bindingProvider.Object); + return await defaultActionSelector.SelectAsync(context); } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/DefaultActionSelectorTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/DefaultActionSelectorTest.cs index 2e662d0162..dd38df11ca 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/DefaultActionSelectorTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/DefaultActionSelectorTest.cs @@ -258,12 +258,12 @@ namespace Microsoft.AspNet.Mvc.Core.Test .Where(a => a.RouteConstraints.Any(c => c.RouteKey == "action" && c.Comparer.Equals(c.RouteValue, action))); } - private static DefaultActionSelector CreateSelector(IEnumerable actions) + private static DefaultActionSelector CreateSelector(IReadOnlyList actions) { - var actionProvider = new Mock>(MockBehavior.Strict); + var actionProvider = new Mock(MockBehavior.Strict); + actionProvider - .Setup(p => p.Invoke(It.IsAny())) - .Callback(c => c.Results.AddRange(actions)); + .Setup(p => p.ActionDescriptors).Returns(new ActionDescriptorsCollection(actions, 0)); var bindingProvider = new Mock(MockBehavior.Strict); bindingProvider diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs index a21d17826e..821127897f 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs @@ -68,6 +68,32 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal(expectedContent, responseContent); } + [Fact] + public async Task ActionDescriptors_CreatedOncePerRequest() + { + // Arrange + var server = TestServer.Create(_provider, _app); + var client = server.Handler; + + var expectedContent = "1"; + + // Call the server 3 times, and make sure the return value remains the same. + var results = new string[3]; + + // Act + for (int i = 0; i < 3; i++) + { + var result = await client.GetAsync("http://localhost/Monitor/CountActionDescriptorInvocations"); + Assert.Equal(200, result.StatusCode); + results[i] = await result.ReadBodyAsStringAsync(); + } + + // Assert + Assert.Equal(expectedContent, results[0]); + Assert.Equal(expectedContent, results[1]); + Assert.Equal(expectedContent, results[2]); + } + // Calculate the path relative to the current application base path. private static string CalculateApplicationBasePath(IApplicationEnvironment appEnvironment) { diff --git a/test/WebSites/BasicWebSite/ActionDescriptorCreationCounter.cs b/test/WebSites/BasicWebSite/ActionDescriptorCreationCounter.cs new file mode 100644 index 0000000000..1be89a7aeb --- /dev/null +++ b/test/WebSites/BasicWebSite/ActionDescriptorCreationCounter.cs @@ -0,0 +1,41 @@ +using System; +using System.Threading; +using Microsoft.AspNet.Mvc; + +namespace BasicWebSite +{ + public class ActionDescriptorCreationCounter : IActionDescriptorProvider + { + private long _callCount; + + public long CallCount + { + get + { + var callCount = Interlocked.Read(ref _callCount); + + return callCount; + } + } + + public int Order + { + get + { + return ReflectedActionDescriptorProvider.DefaultOrder - 100; + } + } + + public void Invoke(ActionDescriptorProviderContext context, Action callNext) + { + callNext(); + + if (context.Results.Count == 0) + { + throw new InvalidOperationException("No actions found!"); + } + + Interlocked.Increment(ref _callCount); + } + } +} \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/BasicWebSite.kproj b/test/WebSites/BasicWebSite/BasicWebSite.kproj index 3d70c1a8d3..70716c2d7e 100644 --- a/test/WebSites/BasicWebSite/BasicWebSite.kproj +++ b/test/WebSites/BasicWebSite/BasicWebSite.kproj @@ -28,6 +28,7 @@ 38820 + diff --git a/test/WebSites/BasicWebSite/Controllers/HomeController.cs b/test/WebSites/BasicWebSite/Controllers/HomeController.cs index 1b9005ca49..0cac77bb59 100644 --- a/test/WebSites/BasicWebSite/Controllers/HomeController.cs +++ b/test/WebSites/BasicWebSite/Controllers/HomeController.cs @@ -7,6 +7,6 @@ namespace BasicWebSite.Controllers public IActionResult Index() { return View(); - } + } } } \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/Controllers/MonitorController.cs b/test/WebSites/BasicWebSite/Controllers/MonitorController.cs new file mode 100644 index 0000000000..7e6e7c2f02 --- /dev/null +++ b/test/WebSites/BasicWebSite/Controllers/MonitorController.cs @@ -0,0 +1,21 @@ +using System.Globalization; +using Microsoft.AspNet.Mvc; +using Microsoft.Framework.DependencyInjection; + +namespace BasicWebSite +{ + public class MonitorController : Controller + { + private readonly ActionDescriptorCreationCounter _counterService; + + public MonitorController(INestedProvider counterService) + { + _counterService = (ActionDescriptorCreationCounter)counterService; + } + + public IActionResult CountActionDescriptorInvocations() + { + return Content(_counterService.CallCount.ToString(CultureInfo.InvariantCulture)); + } + } +} \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/Startup.cs b/test/WebSites/BasicWebSite/Startup.cs index 667b0dc330..ea3e18b75e 100644 --- a/test/WebSites/BasicWebSite/Startup.cs +++ b/test/WebSites/BasicWebSite/Startup.cs @@ -1,4 +1,5 @@ using Microsoft.AspNet.Builder; +using Microsoft.AspNet.Mvc; using Microsoft.AspNet.Routing; using Microsoft.Framework.DependencyInjection; @@ -13,12 +14,14 @@ namespace BasicWebSite { // Add MVC services to the services container services.AddMvc(); + + services.AddSingleton, ActionDescriptorCreationCounter>(); }); // Add MVC to the request pipeline app.UseMvc(routes => { - routes.MapRoute("ActionAsMethod", "{controller}/{action}/{id?}", + routes.MapRoute("ActionAsMethod", "{controller}/{action}", defaults: new { controller = "Home", action = "Index" }); }); }