diff --git a/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs b/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs index 85bf14c1cf..893e8c8df8 100644 --- a/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs +++ b/benchmarks/Microsoft.AspNetCore.Mvc.Performance/MvcEndpointDatasourceBenchmark.cs @@ -111,7 +111,6 @@ namespace Microsoft.AspNetCore.Mvc.Performance var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - Array.Empty(), new MockServiceProvider()); return dataSource; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs index 33d7e6aa93..767aa0c33f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs @@ -165,7 +165,7 @@ namespace Microsoft.Extensions.DependencyInjection services.TryAddEnumerable( ServiceDescriptor.Transient()); - services.TryAddSingleton(); + services.TryAddSingleton(); // // Action Selection diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs new file mode 100644 index 0000000000..633890e08f --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ActionDescriptorCollectionProvider.cs @@ -0,0 +1,33 @@ +// 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 Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + /// + /// A base class for which also provides an + /// for reactive notifications of changes. + /// + /// + /// is used as a base class by the default implementation of + /// . To retrieve an instance of , + /// obtain the from the dependency injection provider and + /// downcast to . + /// + public abstract class ActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider + { + /// + /// Returns the current cached + /// + public abstract ActionDescriptorCollection ActionDescriptors { get; } + + /// + /// Gets an that will be signaled after the + /// collection has changed. + /// + /// The . + public abstract IChangeToken GetChangeToken(); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs new file mode 100644 index 0000000000..6204ce3945 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/DefaultActionDescriptorCollectionProvider.cs @@ -0,0 +1,160 @@ +// 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; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Diagnostics; +using System.Linq; +using System.Threading; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + internal class DefaultActionDescriptorCollectionProvider : ActionDescriptorCollectionProvider + { + private readonly IActionDescriptorProvider[] _actionDescriptorProviders; + private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; + + // The lock is used to protect WRITES to the following (do not need to protect reads once initialized). + private readonly object _lock; + private ActionDescriptorCollection _collection; + private IChangeToken _changeToken; + private CancellationTokenSource _cancellationTokenSource; + private int _version = 0; + + public DefaultActionDescriptorCollectionProvider( + IEnumerable actionDescriptorProviders, + IEnumerable actionDescriptorChangeProviders) + { + _actionDescriptorProviders = actionDescriptorProviders + .OrderBy(p => p.Order) + .ToArray(); + + _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); + + ChangeToken.OnChange( + GetCompositeChangeToken, + UpdateCollection); + + _lock = new object(); + } + + /// + /// Returns a cached collection of . + /// + public override ActionDescriptorCollection ActionDescriptors + { + get + { + Initialize(); + Debug.Assert(_collection != null); + Debug.Assert(_changeToken != null); + + return _collection; + } + } + + /// + /// Gets an that will be signaled after the + /// collection has changed. + /// + /// The . + public override IChangeToken GetChangeToken() + { + Initialize(); + Debug.Assert(_collection != null); + Debug.Assert(_changeToken != null); + + return _changeToken; + } + + private IChangeToken GetCompositeChangeToken() + { + if (_actionDescriptorChangeProviders.Length == 1) + { + return _actionDescriptorChangeProviders[0].GetChangeToken(); + } + + var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; + for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) + { + changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); + } + + return new CompositeChangeToken(changeTokens); + } + + private void Initialize() + { + // Using double-checked locking on initialization because we fire change token callbacks + // when the collection changes. We don't want to do that repeatedly for redundant changes. + // + // The main call path of this code on the first call is async initialization from Endpoint Routing + // which is done in a non-blocking way so in practice no caller will ever block here. + if (_collection == null) + { + lock (_lock) + { + if (_collection == null) + { + UpdateCollection(); + } + } + } + } + + private void UpdateCollection() + { + // Using the lock to initialize writes means that we serialize changes. This eliminates + // the potential for changes to be processed out of order - the risk is that newer data + // could be overwritten by older data. + lock (_lock) + { + var context = new ActionDescriptorProviderContext(); + + for (var i = 0; i < _actionDescriptorProviders.Length; i++) + { + _actionDescriptorProviders[i].OnProvidersExecuting(context); + } + + for (var i = _actionDescriptorProviders.Length - 1; i >= 0; i--) + { + _actionDescriptorProviders[i].OnProvidersExecuted(context); + } + + // The sequence for an update is important because we don't want anyone to obtain + // the new change token but the old action descriptor collection. + // 1. Obtain the old cancellation token source (don't trigger it yet) + // 2. Set the new action descriptor collection + // 3. Set the new change token + // 4. Trigger the old cancellation token source + // + // Consumers who poll will observe a new action descriptor collection at step 2 - they will see + // the new collection and ignore the change token. + // + // Consumers who listen to the change token will requery at step 4 - they will see the new collection + // and new change token. + // + // Anyone who acquires the collection and change token between steps 2 and 3 will be notified of + // a no-op change at step 4. + + // Step 1. + var oldCancellationTokenSource = _cancellationTokenSource; + + // Step 2. + _collection = new ActionDescriptorCollection( + new ReadOnlyCollection(context.Results), + _version++); + + // Step 3. + _cancellationTokenSource = new CancellationTokenSource(); + _changeToken = new CancellationChangeToken(_cancellationTokenSource.Token); + + // Step 4 - might be null if it's the first time. + oldCancellationTokenSource?.Cancel(); + } + } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs index d736512f1c..7b524dd10c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorChangeProvider.cs @@ -1,6 +1,7 @@ // 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 Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.Infrastructure @@ -9,6 +10,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure /// Provides a way to signal invalidation of the cached collection of from an /// . /// + /// + /// The change token returned from is only for use inside the MVC infrastructure. + /// Use to be notified of + /// changes. + /// public interface IActionDescriptorChangeProvider { /// @@ -16,6 +22,11 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure /// instances. /// /// The . + /// + /// The change token returned from is only for use inside the MVC infrastructure. + /// Use to be notified of + /// changes. + /// IChangeToken GetChangeToken(); } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs index 3e64a66677..ee1648e170 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionDescriptorCollectionProvider.cs @@ -1,18 +1,28 @@ // 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 Microsoft.Extensions.Primitives; + namespace Microsoft.AspNetCore.Mvc.Infrastructure { /// /// Provides the currently cached collection of . /// /// + /// /// The default implementation internally caches the collection and uses /// to invalidate this cache, incrementing /// the collection is reconstructed. - /// + /// + /// + /// To be reactively notified of changes, downcast to and + /// subcribe to the change token returned from + /// using . + /// + /// /// 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 IActionDescriptorCollectionProvider { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs deleted file mode 100644 index 49cb219178..0000000000 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionDescriptorCollectionProvider.cs +++ /dev/null @@ -1,95 +0,0 @@ -// 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.Collections.Generic; -using System.Collections.ObjectModel; -using System.Linq; -using System.Threading; -using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Infrastructure; -using Microsoft.Extensions.Primitives; - -namespace Microsoft.AspNetCore.Mvc.Internal -{ - /// - /// Default implementation of . - /// - public class ActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider - { - private readonly IActionDescriptorProvider[] _actionDescriptorProviders; - private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; - private ActionDescriptorCollection _collection; - private int _version = -1; - - /// - /// Initializes a new instance of the class. - /// - /// The sequence of . - /// The sequence of . - public ActionDescriptorCollectionProvider( - IEnumerable actionDescriptorProviders, - IEnumerable actionDescriptorChangeProviders) - { - _actionDescriptorProviders = actionDescriptorProviders - .OrderBy(p => p.Order) - .ToArray(); - - _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); - - ChangeToken.OnChange( - GetCompositeChangeToken, - UpdateCollection); - } - - private IChangeToken GetCompositeChangeToken() - { - if (_actionDescriptorChangeProviders.Length == 1) - { - return _actionDescriptorChangeProviders[0].GetChangeToken(); - } - - var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; - for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) - { - changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); - } - - return new CompositeChangeToken(changeTokens); - } - - /// - /// Returns a cached collection of . - /// - public ActionDescriptorCollection ActionDescriptors - { - get - { - if (_collection == null) - { - UpdateCollection(); - } - - return _collection; - } - } - - private void UpdateCollection() - { - var context = new ActionDescriptorProviderContext(); - - for (var i = 0; i < _actionDescriptorProviders.Length; i++) - { - _actionDescriptorProviders[i].OnProvidersExecuting(context); - } - - for (var i = _actionDescriptorProviders.Length - 1; i >= 0; i--) - { - _actionDescriptorProviders[i].OnProvidersExecuted(context); - } - - _collection = new ActionDescriptorCollection( - new ReadOnlyCollection(context.Results), - Interlocked.Increment(ref _version)); - } - } -} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 2837e6ec4a..560016a4c4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -3,8 +3,10 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Text; +using System.Threading; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; @@ -13,25 +15,27 @@ using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; -using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.Internal { internal class MvcEndpointDataSource : EndpointDataSource { - private readonly object _lock = new object(); private readonly IActionDescriptorCollectionProvider _actions; private readonly MvcEndpointInvokerFactory _invokerFactory; private readonly DefaultHttpContext _httpContextInstance; - private readonly IActionDescriptorChangeProvider[] _actionDescriptorChangeProviders; + // The following are protected by this lock for WRITES only. This pattern is similar + // to DefaultActionDescriptorChangeProvider - see comments there for details on + // all of the threading behaviors. + private readonly object _lock = new object(); private List _endpoints; + private CancellationTokenSource _cancellationTokenSource; + private IChangeToken _changeToken; public MvcEndpointDataSource( IActionDescriptorCollectionProvider actions, MvcEndpointInvokerFactory invokerFactory, - IEnumerable actionDescriptorChangeProviders, IServiceProvider serviceProvider) { if (actions == null) @@ -44,11 +48,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal throw new ArgumentNullException(nameof(invokerFactory)); } - if (actionDescriptorChangeProviders == null) - { - throw new ArgumentNullException(nameof(actionDescriptorChangeProviders)); - } - if (serviceProvider == null) { throw new ArgumentNullException(nameof(serviceProvider)); @@ -56,139 +55,199 @@ namespace Microsoft.AspNetCore.Mvc.Internal _actions = actions; _invokerFactory = invokerFactory; - _actionDescriptorChangeProviders = actionDescriptorChangeProviders.ToArray(); _httpContextInstance = new DefaultHttpContext() { RequestServices = serviceProvider }; ConventionalEndpointInfos = new List(); + + // It's possible for someone to override the collection provider without providing + // change notifications. If that's the case we won't process changes. + if (actions is ActionDescriptorCollectionProvider collectionProviderWithChangeToken) + { + ChangeToken.OnChange( + () => collectionProviderWithChangeToken.GetChangeToken(), + UpdateEndpoints); + } } - private List CreateEndpoints() + public List ConventionalEndpointInfos { get; } + + public override IReadOnlyList Endpoints { - var endpoints = new List(); - StringBuilder patternStringBuilder = null; - - foreach (var action in _actions.ActionDescriptors.Items) + get { - if (action.AttributeRouteInfo == null) + Initialize(); + Debug.Assert(_changeToken != null); + Debug.Assert(_endpoints != null); + return _endpoints; + } + } + + public override IChangeToken GetChangeToken() + { + Initialize(); + Debug.Assert(_changeToken != null); + Debug.Assert(_endpoints != null); + return _changeToken; + } + + private void Initialize() + { + if (_endpoints == null) + { + lock (_lock) { - // In traditional conventional routing setup, the routes defined by a user have a static order - // defined by how they are added into the list. We would like to maintain the same order when building - // up the endpoints too. - // - // Start with an order of '1' for conventional routes as attribute routes have a default order of '0'. - // This is for scenarios dealing with migrating existing Router based code to Endpoint Routing world. - var conventionalRouteOrder = 0; - - // Check each of the conventional patterns to see if the action would be reachable - // If the action and pattern are compatible then create an endpoint with the - // area/controller/action parameter parts replaced with literals - // - // e.g. {controller}/{action} with HomeController.Index and HomeController.Login - // would result in endpoints: - // - Home/Index - // - Home/Login - foreach (var endpointInfo in ConventionalEndpointInfos) + if (_endpoints == null) { - // An 'endpointInfo' is applicable if: - // 1. it has a parameter (or default value) for 'required' non-null route value - // 2. it does not have a parameter (or default value) for 'required' null route value - var isApplicable = true; - foreach (var routeKey in action.RouteValues.Keys) + UpdateEndpoints(); + } + } + } + } + + private void UpdateEndpoints() + { + lock (_lock) + { + var endpoints = new List(); + StringBuilder patternStringBuilder = null; + + foreach (var action in _actions.ActionDescriptors.Items) + { + if (action.AttributeRouteInfo == null) + { + // In traditional conventional routing setup, the routes defined by a user have a static order + // defined by how they are added into the list. We would like to maintain the same order when building + // up the endpoints too. + // + // Start with an order of '1' for conventional routes as attribute routes have a default order of '0'. + // This is for scenarios dealing with migrating existing Router based code to Endpoint Routing world. + var conventionalRouteOrder = 0; + + // Check each of the conventional patterns to see if the action would be reachable + // If the action and pattern are compatible then create an endpoint with the + // area/controller/action parameter parts replaced with literals + // + // e.g. {controller}/{action} with HomeController.Index and HomeController.Login + // would result in endpoints: + // - Home/Index + // - Home/Login + foreach (var endpointInfo in ConventionalEndpointInfos) { - if (!MatchRouteValue(action, endpointInfo, routeKey)) + // An 'endpointInfo' is applicable if: + // 1. it has a parameter (or default value) for 'required' non-null route value + // 2. it does not have a parameter (or default value) for 'required' null route value + var isApplicable = true; + foreach (var routeKey in action.RouteValues.Keys) { - isApplicable = false; - break; - } - } - - if (!isApplicable) - { - continue; - } - - var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); - - for (var i = 0; i < newPathSegments.Count; i++) - { - // Check if the pattern can be shortened because the remaining parameters are optional - // - // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index - // can resolve to the following endpoints: - // - /Home/Index/{id?} - // - /Home - // - / - if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) - { - var subPathSegments = newPathSegments.Take(i); - - var subEndpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, subPathSegments), - subPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); - endpoints.Add(subEndpoint); - } - - List segmentParts = null; // Initialize only as needed - var segment = newPathSegments[i]; - for (var j = 0; j < segment.Parts.Count; j++) - { - var part = segment.Parts[j]; - - if (part.IsParameter && - part is RoutePatternParameterPart parameterPart && - action.RouteValues.ContainsKey(parameterPart.Name)) + if (!MatchRouteValue(action, endpointInfo, routeKey)) { - if (segmentParts == null) - { - segmentParts = segment.Parts.ToList(); - } - - // Replace parameter with literal value - segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + isApplicable = false; + break; } } - // A parameter part was replaced so replace segment with updated parts - if (segmentParts != null) + if (!isApplicable) { - newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + continue; } - } + var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); + + for (var i = 0; i < newPathSegments.Count; i++) + { + // Check if the pattern can be shortened because the remaining parameters are optional + // + // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index + // can resolve to the following endpoints: + // - /Home/Index/{id?} + // - /Home + // - / + if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + { + var subPathSegments = newPathSegments.Take(i); + + var subEndpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, subPathSegments), + subPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(subEndpoint); + } + + List segmentParts = null; // Initialize only as needed + var segment = newPathSegments[i]; + for (var j = 0; j < segment.Parts.Count; j++) + { + var part = segment.Parts[j]; + + if (part.IsParameter && + part is RoutePatternParameterPart parameterPart && + action.RouteValues.ContainsKey(parameterPart.Name)) + { + if (segmentParts == null) + { + segmentParts = segment.Parts.ToList(); + } + + // Replace parameter with literal value + segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + } + } + + // A parameter part was replaced so replace segment with updated parts + if (segmentParts != null) + { + newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + } + } + + var endpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, newPathSegments), + newPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(endpoint); + } + } + else + { var endpoint = CreateEndpoint( action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, newPathSegments), - newPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); + action.AttributeRouteInfo.Name, + action.AttributeRouteInfo.Template, + RoutePatternFactory.Parse(action.AttributeRouteInfo.Template).PathSegments, + nonInlineDefaults: null, + action.AttributeRouteInfo.Order, + action.AttributeRouteInfo, + suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration); endpoints.Add(endpoint); } } - else - { - var endpoint = CreateEndpoint( - action, - action.AttributeRouteInfo.Name, - action.AttributeRouteInfo.Template, - RoutePatternFactory.Parse(action.AttributeRouteInfo.Template).PathSegments, - nonInlineDefaults: null, - action.AttributeRouteInfo.Order, - action.AttributeRouteInfo, - suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration); - endpoints.Add(endpoint); - } - } - return endpoints; + // See comments in DefaultActionDescriptorCollectionProvider. These steps are done + // in a specific order to ensure callers always see a consistent state. + + // Step 1 - capture old token + var oldCancellationTokenSource = _cancellationTokenSource; + + // Step 2 - update endpoints + _endpoints = endpoints; + + // Step 3 - create new change token + _cancellationTokenSource = new CancellationTokenSource(); + _changeToken = new CancellationChangeToken(_cancellationTokenSource.Token); + + // Step 4 - trigger old token + oldCancellationTokenSource?.Cancel(); + } string GetPattern(ref StringBuilder sb, IEnumerable segments) { @@ -442,49 +501,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private IChangeToken GetCompositeChangeToken() - { - if (_actionDescriptorChangeProviders.Length == 1) - { - return _actionDescriptorChangeProviders[0].GetChangeToken(); - } - - var changeTokens = new IChangeToken[_actionDescriptorChangeProviders.Length]; - for (var i = 0; i < _actionDescriptorChangeProviders.Length; i++) - { - changeTokens[i] = _actionDescriptorChangeProviders[i].GetChangeToken(); - } - - return new CompositeChangeToken(changeTokens); - } - - public override IChangeToken GetChangeToken() => NullChangeToken.Singleton; - - public override IReadOnlyList Endpoints - { - get - { - // Want to initialize endpoints once and then cache while ensuring a null collection is never returned - // Local copy for thread safety + double check locking - var localEndpoints = _endpoints; - if (localEndpoints == null) - { - lock (_lock) - { - localEndpoints = _endpoints; - if (localEndpoints == null) - { - _endpoints = localEndpoints = CreateEndpoints(); - } - } - } - - return localEndpoints; - } - } - - public List ConventionalEndpointInfos { get; } - private class SuppressLinkGenerationMetadata : ISuppressLinkGenerationMetadata { } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs similarity index 70% rename from test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs rename to test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs index 0c599b47c6..cac4f58aec 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionDescriptorCollectionProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/DefaultActionDescriptorCollectionProviderTest.cs @@ -4,14 +4,13 @@ using System.Linq; using System.Threading; using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.Primitives; using Moq; using Xunit; -namespace Microsoft.AspNetCore.Mvc.Internal +namespace Microsoft.AspNetCore.Mvc.Infrastructure { - public class ActionDescriptorCollectionProviderTest + public class DefaultActionDescriptorCollectionProviderTest { [Fact] public void ActionDescriptors_ReadsDescriptorsFromActionDescriptorProviders() @@ -24,7 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var expected3 = new ActionDescriptor(); var actionDescriptorProvider2 = GetActionDescriptorProvider(expected2, expected3); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider1, actionDescriptorProvider2 }, Enumerable.Empty()); @@ -46,7 +45,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Arrange var actionDescriptorProvider = GetActionDescriptorProvider(new ActionDescriptor()); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider }, Enumerable.Empty()); @@ -66,55 +65,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void ActionDescriptors_UpdateWhenChangeTokenProviderChanges() - { - // Arrange - var actionDescriptorProvider = new Mock(); - var expected1 = new ActionDescriptor(); - var expected2 = new ActionDescriptor(); - - var invocations = 0; - actionDescriptorProvider - .Setup(p => p.OnProvidersExecuting(It.IsAny())) - .Callback((ActionDescriptorProviderContext context) => - { - if (invocations == 0) - { - context.Results.Add(expected1); - } - else - { - context.Results.Add(expected2); - } - - invocations++; - }); - var changeProvider = new TestChangeProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( - new[] { actionDescriptorProvider.Object }, - new[] { changeProvider }); - - // Act - 1 - var collection1 = actionDescriptorCollectionProvider.ActionDescriptors; - - // Assert - 1 - Assert.Equal(0, collection1.Version); - Assert.Collection(collection1.Items, - item => Assert.Same(expected1, item)); - - // Act - 2 - changeProvider.TokenSource.Cancel(); - var collection2 = actionDescriptorCollectionProvider.ActionDescriptors; - - // Assert - 2 - Assert.NotSame(collection1, collection2); - Assert.Equal(1, collection2.Version); - Assert.Collection(collection2.Items, - item => Assert.Same(expected2, item)); - } - - [Fact] - public void ActionDescriptors_SubscribesToNewChangeNotificationsAfterInvalidating() + public void ActionDescriptors_UpdatesAndResubscripes_WhenChangeTokenTriggers() { // Arrange var actionDescriptorProvider = new Mock(); @@ -143,34 +94,61 @@ namespace Microsoft.AspNetCore.Mvc.Internal invocations++; }); var changeProvider = new TestChangeProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider.Object }, new[] { changeProvider }); // Act - 1 + var changeToken1 = actionDescriptorCollectionProvider.GetChangeToken(); var collection1 = actionDescriptorCollectionProvider.ActionDescriptors; + ActionDescriptorCollection captured = null; + changeToken1.RegisterChangeCallback((_) => + { + captured = actionDescriptorCollectionProvider.ActionDescriptors; + }, null); + // Assert - 1 + Assert.False(changeToken1.HasChanged); Assert.Equal(0, collection1.Version); Assert.Collection(collection1.Items, item => Assert.Same(expected1, item)); // Act - 2 changeProvider.TokenSource.Cancel(); + var changeToken2 = actionDescriptorCollectionProvider.GetChangeToken(); var collection2 = actionDescriptorCollectionProvider.ActionDescriptors; + changeToken2.RegisterChangeCallback((_) => + { + captured = actionDescriptorCollectionProvider.ActionDescriptors; + }, null); + // Assert - 2 + Assert.NotSame(changeToken1, changeToken2); + Assert.True(changeToken1.HasChanged); + Assert.False(changeToken2.HasChanged); + Assert.NotSame(collection1, collection2); + Assert.NotNull(captured); + Assert.Same(captured, collection2); Assert.Equal(1, collection2.Version); Assert.Collection(collection2.Items, item => Assert.Same(expected2, item)); // Act - 3 changeProvider.TokenSource.Cancel(); + var changeToken3 = actionDescriptorCollectionProvider.GetChangeToken(); var collection3 = actionDescriptorCollectionProvider.ActionDescriptors; // Assert - 3 + Assert.NotSame(changeToken2, changeToken3); + Assert.True(changeToken2.HasChanged); + Assert.False(changeToken3.HasChanged); + Assert.NotSame(collection2, collection3); + Assert.NotNull(captured); + Assert.Same(captured, collection3); Assert.Equal(2, collection3.Version); Assert.Collection(collection3.Items, item => Assert.Same(expected3, item)); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs index 0187f81666..c4374a5bbb 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionConstraintCacheTest.cs @@ -159,7 +159,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static ActionConstraintCache CreateCache(params IActionConstraintProvider[] providers) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, providers); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs index 5835a579f0..5b3adffcbc 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ActionSelectorTest.cs @@ -932,7 +932,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure private ControllerActionDescriptor InvokeActionSelector(RouteContext context) { var actionDescriptorProvider = GetActionDescriptorProvider(); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionDescriptorProvider }, Enumerable.Empty()); @@ -1092,7 +1092,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure private static ActionConstraintCache GetActionConstraintCache(IActionConstraintProvider[] actionConstraintProviders = null) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, actionConstraintProviders.AsEnumerable() ?? new List()); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index 416e359291..be31acba0b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -133,47 +133,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.True(actionInvokerCalled); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] - public void GetChangeToken_MultipleChangeTokenProviders_ComposedResult() - { - // Arrange - var featureCollection = new FeatureCollection(); - featureCollection.Set(new EndpointFeature - { - RouteValues = new RouteValueDictionary() - }); - - var httpContextMock = new Mock(); - httpContextMock.Setup(m => m.Features).Returns(featureCollection); - - var descriptorProviderMock = new Mock(); - descriptorProviderMock.Setup(m => m.ActionDescriptors).Returns(new ActionDescriptorCollection(new List(), 0)); - - var actionInvokerMock = new Mock(); - - var actionInvokerProviderMock = new Mock(); - actionInvokerProviderMock.Setup(m => m.CreateInvoker(It.IsAny())).Returns(actionInvokerMock.Object); - - var changeTokenMock = new Mock(); - - var changeProvider1Mock = new Mock(); - changeProvider1Mock.Setup(m => m.GetChangeToken()).Returns(changeTokenMock.Object); - var changeProvider2Mock = new Mock(); - changeProvider2Mock.Setup(m => m.GetChangeToken()).Returns(changeTokenMock.Object); - - var dataSource = CreateMvcEndpointDataSource( - descriptorProviderMock.Object, - new MvcEndpointInvokerFactory(actionInvokerProviderMock.Object), - new[] { changeProvider1Mock.Object, changeProvider2Mock.Object }); - - // Act - var changeToken = dataSource.GetChangeToken(); - - // Assert - var compositeChangeToken = Assert.IsType(changeToken); - Assert.Equal(2, compositeChangeToken.ChangeTokens.Count); - } - [Theory] [InlineData("{controller}/{action}/{id?}", new[] { "TestController/TestAction/{id?}" })] [InlineData("{controller}/{id?}", new string[] { })] @@ -287,11 +246,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal actionDescriptorCollectionProviderMock.VerifyGet(m => m.ActionDescriptors, Times.Once); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] + [Fact] public void Endpoints_ChangeTokenTriggered_EndpointsRecreated() { // Arrange - var actionDescriptorCollectionProviderMock = new Mock(); + var actionDescriptorCollectionProviderMock = new Mock(); actionDescriptorCollectionProviderMock .Setup(m => m.ActionDescriptors) .Returns(new ActionDescriptorCollection(new[] @@ -300,19 +259,18 @@ namespace Microsoft.AspNetCore.Mvc.Internal }, version: 0)); CancellationTokenSource cts = null; + actionDescriptorCollectionProviderMock + .Setup(m => m.GetChangeToken()) + .Returns(() => + { + cts = new CancellationTokenSource(); + var changeToken = new CancellationChangeToken(cts.Token); - var changeProviderMock = new Mock(); - changeProviderMock.Setup(m => m.GetChangeToken()).Returns(() => - { - cts = new CancellationTokenSource(); - var changeToken = new CancellationChangeToken(cts.Token); + return changeToken; + }); - return changeToken; - }); - var dataSource = CreateMvcEndpointDataSource( - actionDescriptorCollectionProviderMock.Object, - actionDescriptorChangeProviders: new[] { changeProviderMock.Object }); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollectionProviderMock.Object); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo( string.Empty, "{controller}/{action}", @@ -752,15 +710,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal private MvcEndpointDataSource CreateMvcEndpointDataSource( IActionDescriptorCollectionProvider actionDescriptorCollectionProvider = null, - MvcEndpointInvokerFactory mvcEndpointInvokerFactory = null, - IEnumerable actionDescriptorChangeProviders = null) + MvcEndpointInvokerFactory mvcEndpointInvokerFactory = null) { if (actionDescriptorCollectionProvider == null) { - var mockDescriptorProvider = new Mock(); - mockDescriptorProvider.Setup(m => m.ActionDescriptors).Returns(new ActionDescriptorCollection(new List(), 0)); - - actionDescriptorCollectionProvider = mockDescriptorProvider.Object; + actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( + Array.Empty(), + Array.Empty()); } var serviceProviderMock = new Mock(); @@ -769,7 +725,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal var dataSource = new MvcEndpointDataSource( actionDescriptorCollectionProvider, mvcEndpointInvokerFactory ?? new MvcEndpointInvokerFactory(new ActionInvokerFactory(Array.Empty())), - actionDescriptorChangeProviders ?? Array.Empty(), serviceProviderMock.Object); return dataSource; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs index 298786c121..5936d34ef6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -351,7 +351,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing } }); - var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var actionDescriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new IActionDescriptorProvider[] { actionDescriptorProvider.Object, }, Enumerable.Empty()); @@ -398,7 +398,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing private static ActionConstraintCache GetActionConstraintCache(IActionConstraintProvider[] actionConstraintProviders = null) { - var descriptorProvider = new ActionDescriptorCollectionProvider( + var descriptorProvider = new DefaultActionDescriptorCollectionProvider( Enumerable.Empty(), Enumerable.Empty()); return new ActionConstraintCache(descriptorProvider, actionConstraintProviders.AsEnumerable() ?? new List()); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs index e6ee1638f6..1e1d1f8951 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/KnownRouteValueConstraintTests.cs @@ -173,7 +173,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing .Setup(p => p.OnProvidersExecuted(It.IsAny())) .Verifiable(); - var descriptorCollectionProvider = new ActionDescriptorCollectionProvider( + var descriptorCollectionProvider = new DefaultActionDescriptorCollectionProvider( new[] { actionProvider.Object }, Enumerable.Empty()); diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs index 40e556ad72..d216ad4aff 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs @@ -1087,7 +1087,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests }); } - [Fact(Skip = "https://github.com/aspnet/Routing/issues/722")] + [Fact] public async Task ApiExplorer_Updates_WhenActionDescriptorCollectionIsUpdated() { // Act - 1