From 1196349bf497516c330aad2cb9ca2eb2935bb287 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Wed, 11 Jul 2018 08:47:05 -0700 Subject: [PATCH] [Fixes #583] Handle change events in RouteValueBaseEndpointFinder --- .../CompositeEndpointDataSource.cs | 52 +++- .../RouteValuesBasedEndpointFinder.cs | 30 +- .../CompositeEndpointDataSourceTest.cs | 183 +++++++++++++ .../RouteValueBasedEndpointFinderTest.cs | 256 ++++++++++++++++++ .../TestObjects/DynamicEndpointDataSource.cs | 53 ++++ 5 files changed, 563 insertions(+), 11 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Routing.Tests/CompositeEndpointDataSourceTest.cs create mode 100644 test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs create mode 100644 test/Microsoft.AspNetCore.Routing.Tests/TestObjects/DynamicEndpointDataSource.cs diff --git a/src/Microsoft.AspNetCore.Routing/CompositeEndpointDataSource.cs b/src/Microsoft.AspNetCore.Routing/CompositeEndpointDataSource.cs index 93fd595ede..75e5360d47 100644 --- a/src/Microsoft.AspNetCore.Routing/CompositeEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Routing/CompositeEndpointDataSource.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Routing @@ -12,9 +13,9 @@ namespace Microsoft.AspNetCore.Routing { private readonly EndpointDataSource[] _dataSources; private readonly object _lock; - - private IChangeToken _changeToken; private IReadOnlyList _endpoints; + private IChangeToken _consumerChangeToken; + private CancellationTokenSource _cts; internal CompositeEndpointDataSource(IEnumerable dataSources) { @@ -23,6 +24,7 @@ namespace Microsoft.AspNetCore.Routing throw new ArgumentNullException(nameof(dataSources)); } + CreateChangeToken(); _dataSources = dataSources.ToArray(); _lock = new object(); } @@ -32,7 +34,7 @@ namespace Microsoft.AspNetCore.Routing get { EnsureInitialized(); - return _changeToken; + return _consumerChangeToken; } } @@ -48,7 +50,7 @@ namespace Microsoft.AspNetCore.Routing // Defer initialization to avoid doing lots of reflection on startup. private void EnsureInitialized() { - if (_changeToken == null) + if (_endpoints == null) { Initialize(); } @@ -60,11 +62,49 @@ namespace Microsoft.AspNetCore.Routing { lock (_lock) { - _changeToken = new CompositeChangeToken(_dataSources.Select(d => d.ChangeToken).ToArray()); + if (_endpoints == null) + { + _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); + + foreach (var dataSource in _dataSources) + { + Extensions.Primitives.ChangeToken.OnChange( + () => dataSource.ChangeToken, + () => HandleChange()); + } + } + } + } + + private void HandleChange() + { + lock (_lock) + { + // Refresh the endpoints from datasource so that callbacks can get the latest endpoints _endpoints = _dataSources.SelectMany(d => d.Endpoints).ToArray(); - _changeToken.RegisterChangeCallback((state) => Initialize(), null); + // Prevent consumers from re-registering callback to inflight events as that can + // cause a stackoverflow + // Example: + // 1. B registers A + // 2. A fires event causing B's callback to get called + // 3. B executes some code in its callback, but needs to re-register callback + // in the same callback + var oldTokenSource = _cts; + var oldToken = _consumerChangeToken; + + CreateChangeToken(); + + // Raise consumer callbacks. Any new callback registration would happen on the new token + // created in earlier step. + oldTokenSource.Cancel(); } } + + private void CreateChangeToken() + { + _cts = new CancellationTokenSource(); + _consumerChangeToken = new CancellationChangeToken(_cts.Token); + } } } diff --git a/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs b/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs index 0b0fee9edd..d9add483aa 100644 --- a/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs +++ b/src/Microsoft.AspNetCore.Routing/RouteValuesBasedEndpointFinder.cs @@ -27,7 +27,13 @@ namespace Microsoft.AspNetCore.Routing _endpointDataSource = endpointDataSource; _objectPool = objectPool; + // Build initial matches BuildOutboundMatches(); + + // Register for changes in endpoints + Extensions.Primitives.ChangeToken.OnChange( + () => _endpointDataSource.ChangeToken, + () => HandleChange()); } public IEnumerable FindEndpoints(RouteValuesBasedEndpointFinderContext context) @@ -56,14 +62,28 @@ namespace Microsoft.AspNetCore.Routing .Select(match => (MatcherEndpoint)match.Entry.Data); } - private void BuildOutboundMatches() + private void HandleChange() { - var (allOutboundMatches, namedOutboundMatches) = GetOutboundMatches(); - _namedMatches = GetNamedMatches(namedOutboundMatches); - _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allOutboundMatches.ToArray()); + // rebuild the matches + BuildOutboundMatches(); + + // re-register the callback as the change token is one time use only and a new change token + // is produced every time + Extensions.Primitives.ChangeToken.OnChange( + () => _endpointDataSource.ChangeToken, + () => HandleChange()); } - private (IEnumerable, IDictionary>) GetOutboundMatches() + private void BuildOutboundMatches() + { + // Refresh the matches in the case where a datasource's endpoints changes. The following is OK to do + // as refresh of new endpoints happens within a lock and also these fields are not publicly accessible. + var (allMatches, namedMatches) = GetOutboundMatches(); + _namedMatches = GetNamedMatches(namedMatches); + _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allMatches.ToArray()); + } + + protected virtual (IEnumerable, IDictionary>) GetOutboundMatches() { var allOutboundMatches = new List(); var namedOutboundMatches = new Dictionary>(StringComparer.OrdinalIgnoreCase); diff --git a/test/Microsoft.AspNetCore.Routing.Tests/CompositeEndpointDataSourceTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/CompositeEndpointDataSourceTest.cs new file mode 100644 index 0000000000..6f5978af52 --- /dev/null +++ b/test/Microsoft.AspNetCore.Routing.Tests/CompositeEndpointDataSourceTest.cs @@ -0,0 +1,183 @@ +// 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.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Routing.Matchers; +using Microsoft.AspNetCore.Routing.TestObjects; +using Microsoft.Extensions.Primitives; +using Xunit; + +namespace Microsoft.AspNetCore.Routing +{ + public class CompositeEndpointDataSourceTest + { + [Fact] + public void CreatesShallowCopyOf_ListOfEndpoints() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/b"); + var dataSource = new DefaultEndpointDataSource(new Endpoint[] { endpoint1, endpoint2 }); + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource }); + + // Act + var endpoints = compositeDataSource.Endpoints; + + // Assert + Assert.NotSame(endpoints, dataSource.Endpoints); + Assert.Equal(endpoints, dataSource.Endpoints); + } + + [Fact] + public void Endpoints_ReturnsAllEndpoints_FromMultipleDataSources() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/b"); + var endpoint3 = CreateEndpoint("/c"); + var endpoint4 = CreateEndpoint("/d"); + var endpoint5 = CreateEndpoint("/e"); + var compositeDataSource = new CompositeEndpointDataSource(new[] + { + new DefaultEndpointDataSource(new Endpoint[] { endpoint1, endpoint2 }), + new DefaultEndpointDataSource(new Endpoint[] { endpoint3, endpoint4 }), + new DefaultEndpointDataSource(new Endpoint[] { endpoint5 }), + }); + + // Act + var endpoints = compositeDataSource.Endpoints; + + // Assert + Assert.Collection( + endpoints, + (ep) => Assert.Same(endpoint1, ep), + (ep) => Assert.Same(endpoint2, ep), + (ep) => Assert.Same(endpoint3, ep), + (ep) => Assert.Same(endpoint4, ep), + (ep) => Assert.Same(endpoint5, ep)); + } + + [Fact] + public void DataSourceChanges_AreReflected_InEndpoints() + { + // Arrange1 + var endpoint1 = CreateEndpoint("/a"); + var dataSource1 = new DynamicEndpointDataSource(endpoint1); + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource1 }); + + // Act1 + var endpoints = compositeDataSource.Endpoints; + + // Assert1 + var endpoint = Assert.Single(endpoints); + Assert.Same(endpoint1, endpoint); + + // Arrange2 + var endpoint2 = CreateEndpoint("/b"); + + // Act2 + dataSource1.AddEndpoint(endpoint2); + + // Assert2 + Assert.Collection( + compositeDataSource.Endpoints, + (ep) => Assert.Same(endpoint1, ep), + (ep) => Assert.Same(endpoint2, ep)); + + // Arrange3 + var endpoint3 = CreateEndpoint("/c"); + + // Act2 + dataSource1.AddEndpoint(endpoint3); + + // Assert2 + Assert.Collection( + compositeDataSource.Endpoints, + (ep) => Assert.Same(endpoint1, ep), + (ep) => Assert.Same(endpoint2, ep), + (ep) => Assert.Same(endpoint3, ep)); + } + + [Fact] + public void ConsumerChangeToken_IsRefreshed_WhenDataSourceCallbackFires() + { + // Arrange1 + var endpoint1 = CreateEndpoint("/a"); + var dataSource1 = new DynamicEndpointDataSource(endpoint1); + var compositeDataSource = new CompositeEndpointDataSource(new[] { dataSource1 }); + + // Act1 + var endpoints = compositeDataSource.Endpoints; + + // Assert1 + var changeToken1 = compositeDataSource.ChangeToken; + var token = Assert.IsType(changeToken1); + Assert.False(token.HasChanged); // initial state + + // Arrange2 + var endpoint2 = CreateEndpoint("/b"); + + // Act2 + dataSource1.AddEndpoint(endpoint2); + + // Assert2 + Assert.True(changeToken1.HasChanged); // old token is expected to be changed + var changeToken2 = compositeDataSource.ChangeToken; // new token is in a unchanged state + Assert.NotSame(changeToken2, changeToken1); + token = Assert.IsType(changeToken2); + Assert.False(token.HasChanged); + + // Arrange3 + var endpoint3 = CreateEndpoint("/c"); + + // Act2 + dataSource1.AddEndpoint(endpoint3); + + // Assert2 + Assert.True(changeToken2.HasChanged); // old token is expected to be changed + var changeToken3 = compositeDataSource.ChangeToken; // new token is in a unchanged state + Assert.NotSame(changeToken3, changeToken2); + Assert.NotSame(changeToken3, changeToken1); + token = Assert.IsType(changeToken3); + Assert.False(token.HasChanged); + } + + private MatcherEndpoint CreateEndpoint( + string template, + object defaultValues = null, + object requiredValues = null, + int order = 0, + string routeName = null) + { + var defaults = defaultValues == null ? new RouteValueDictionary() : new RouteValueDictionary(defaultValues); + var required = requiredValues == null ? new RouteValueDictionary() : new RouteValueDictionary(requiredValues); + + return new MatcherEndpoint( + next => (httpContext) => Task.CompletedTask, + template, + defaults, + required, + order, + EndpointMetadataCollection.Empty, + null); + } + + private class CustomEndpointDataSource : EndpointDataSource + { + private readonly CancellationTokenSource _cts; + private readonly CancellationChangeToken _token; + + public CustomEndpointDataSource() + { + _cts = new CancellationTokenSource(); + _token = new CancellationChangeToken(_cts.Token); + } + + public override IChangeToken ChangeToken => _token; + public override IReadOnlyList Endpoints => Array.Empty(); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs new file mode 100644 index 0000000000..aff35c2fa3 --- /dev/null +++ b/test/Microsoft.AspNetCore.Routing.Tests/RouteValueBasedEndpointFinderTest.cs @@ -0,0 +1,256 @@ +// 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.Linq; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Routing.Internal; +using Microsoft.AspNetCore.Routing.Matchers; +using Microsoft.AspNetCore.Routing.TestObjects; +using Microsoft.AspNetCore.Routing.Tree; +using Microsoft.Extensions.ObjectPool; +using Microsoft.Extensions.Options; +using Xunit; + +namespace Microsoft.AspNetCore.Routing +{ + public class RouteValueBasedEndpointFinderTest + { + [Fact] + public void GetOutboundMatches_GetsNamedMatchesFor_EndpointsHaving_IRouteNameMetadata() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/a", routeName: "named"); + + // Act + var finder = CreateEndpointFinder(endpoint1, endpoint2); + + // Assert + Assert.NotNull(finder.AllMatches); + Assert.Equal(2, finder.AllMatches.Count()); + Assert.NotNull(finder.NamedMatches); + Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches)); + var namedMatch = Assert.Single(namedMatches); + var actual = Assert.IsType(namedMatch.Entry.Data); + Assert.Same(endpoint2, actual); + } + + [Fact] + public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/a", routeName: "named"); + var endpoint3 = CreateEndpoint("/b", routeName: "named"); + + // Act + var finder = CreateEndpointFinder(endpoint1, endpoint2, endpoint3); + + // Assert + Assert.NotNull(finder.AllMatches); + Assert.Equal(3, finder.AllMatches.Count()); + Assert.NotNull(finder.NamedMatches); + Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches)); + Assert.Equal(2, namedMatches.Count); + Assert.Same(endpoint2, Assert.IsType(namedMatches[0].Entry.Data)); + Assert.Same(endpoint3, Assert.IsType(namedMatches[1].Entry.Data)); + } + + [Fact] + public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName_IgnoringCase() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/a", routeName: "named"); + var endpoint3 = CreateEndpoint("/b", routeName: "NaMed"); + + // Act + var finder = CreateEndpointFinder(endpoint1, endpoint2, endpoint3); + + // Assert + Assert.NotNull(finder.AllMatches); + Assert.Equal(3, finder.AllMatches.Count()); + Assert.NotNull(finder.NamedMatches); + Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches)); + Assert.Equal(2, namedMatches.Count); + Assert.Same(endpoint2, Assert.IsType(namedMatches[0].Entry.Data)); + Assert.Same(endpoint3, Assert.IsType(namedMatches[1].Entry.Data)); + } + + [Fact] + public void GetOutboundMatches_DoesNotGetNamedMatchesFor_EndpointsHaving_INameMetadata() + { + // Arrange + var endpoint1 = CreateEndpoint("/a"); + var endpoint2 = CreateEndpoint("/a", routeName: "named"); + var endpoint3 = CreateEndpoint( + "/b", + metadataCollection: new EndpointMetadataCollection(new[] { new NameMetadata("named") })); + + // Act + var finder = CreateEndpointFinder(endpoint1, endpoint2); + + // Assert + Assert.NotNull(finder.AllMatches); + Assert.Equal(2, finder.AllMatches.Count()); + Assert.NotNull(finder.NamedMatches); + Assert.True(finder.NamedMatches.TryGetValue("named", out var namedMatches)); + var namedMatch = Assert.Single(namedMatches); + var actual = Assert.IsType(namedMatch.Entry.Data); + Assert.Same(endpoint2, actual); + } + + [Fact] + public void EndpointDataSource_ChangeCallback_Refreshes_OutboundMatches() + { + // Arrange 1 + var endpoint1 = CreateEndpoint("/a"); + var dynamicDataSource = new DynamicEndpointDataSource(new[] { endpoint1 }); + var objectPoolProvider = new DefaultObjectPoolProvider(); + var objectPool = objectPoolProvider.Create(new UriBuilderContextPooledObjectPolicy()); + + // Act 1 + var finder = new CustomRouteValuesBasedEndpointFinder( + new CompositeEndpointDataSource(new[] { dynamicDataSource }), + objectPool); + + // Assert 1 + Assert.NotNull(finder.AllMatches); + var match = Assert.Single(finder.AllMatches); + var actual = Assert.IsType(match.Entry.Data); + Assert.Same(endpoint1, actual); + + // Arrange 2 + var endpoint2 = CreateEndpoint("/b"); + + // Act 2 + // Trigger change + dynamicDataSource.AddEndpoint(endpoint2); + + // Arrange 2 + var endpoint3 = CreateEndpoint("/c"); + + // Act 2 + // Trigger change + dynamicDataSource.AddEndpoint(endpoint3); + + // Arrange 3 + var endpoint4 = CreateEndpoint("/d"); + + // Act 3 + // Trigger change + dynamicDataSource.AddEndpoint(endpoint4); + + // Assert 3 + Assert.NotNull(finder.AllMatches); + Assert.Collection( + finder.AllMatches, + (m) => + { + actual = Assert.IsType(m.Entry.Data); + Assert.Same(endpoint1, actual); + }, + (m) => + { + actual = Assert.IsType(m.Entry.Data); + Assert.Same(endpoint2, actual); + }, + (m) => + { + actual = Assert.IsType(m.Entry.Data); + Assert.Same(endpoint3, actual); + }, + (m) => + { + actual = Assert.IsType(m.Entry.Data); + Assert.Same(endpoint4, actual); + }); + } + + private CustomRouteValuesBasedEndpointFinder CreateEndpointFinder(params Endpoint[] endpoints) + { + return CreateEndpointFinder(new DefaultEndpointDataSource(endpoints)); + } + + private CustomRouteValuesBasedEndpointFinder CreateEndpointFinder(params EndpointDataSource[] endpointDataSources) + { + var objectPoolProvider = new DefaultObjectPoolProvider(); + var objectPool = objectPoolProvider.Create(new UriBuilderContextPooledObjectPolicy()); + + return new CustomRouteValuesBasedEndpointFinder( + new CompositeEndpointDataSource(endpointDataSources), + objectPool); + } + + private MatcherEndpoint CreateEndpoint( + string template, + object defaultValues = null, + object requiredValues = null, + int order = 0, + string routeName = null, + EndpointMetadataCollection metadataCollection = null) + { + var defaults = defaultValues == null ? new RouteValueDictionary() : new RouteValueDictionary(defaultValues); + var required = requiredValues == null ? new RouteValueDictionary() : new RouteValueDictionary(requiredValues); + + if (metadataCollection == null) + { + metadataCollection = EndpointMetadataCollection.Empty; + if (!string.IsNullOrEmpty(routeName)) + { + metadataCollection = new EndpointMetadataCollection(new[] { new RouteNameMetadata(routeName) }); + } + } + + return new MatcherEndpoint( + next => (httpContext) => Task.CompletedTask, + template, + defaults, + required, + order, + metadataCollection, + null); + } + + private class RouteNameMetadata : IRouteNameMetadata + { + public RouteNameMetadata(string name) + { + Name = name; + } + public string Name { get; } + } + + private class NameMetadata : INameMetadata + { + public NameMetadata(string name) + { + Name = name; + } + public string Name { get; } + } + + private class CustomRouteValuesBasedEndpointFinder : RouteValuesBasedEndpointFinder + { + public CustomRouteValuesBasedEndpointFinder( + CompositeEndpointDataSource endpointDataSource, + ObjectPool objectPool) + : base(endpointDataSource, objectPool) + { + } + + public IEnumerable AllMatches { get; private set; } + + public IDictionary> NamedMatches { get; private set; } + + protected override (IEnumerable, IDictionary>) GetOutboundMatches() + { + var matches = base.GetOutboundMatches(); + AllMatches = matches.Item1; + NamedMatches = matches.Item2; + return matches; + } + } + } +} diff --git a/test/Microsoft.AspNetCore.Routing.Tests/TestObjects/DynamicEndpointDataSource.cs b/test/Microsoft.AspNetCore.Routing.Tests/TestObjects/DynamicEndpointDataSource.cs new file mode 100644 index 0000000000..d434b9c924 --- /dev/null +++ b/test/Microsoft.AspNetCore.Routing.Tests/TestObjects/DynamicEndpointDataSource.cs @@ -0,0 +1,53 @@ +// 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.Threading; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Routing.TestObjects +{ + public class DynamicEndpointDataSource : EndpointDataSource + { + private readonly List _endpoints; + private CancellationTokenSource _cts; + private CancellationChangeToken _changeToken; + private readonly object _lock; + + public DynamicEndpointDataSource(params Endpoint[] endpoints) + { + _endpoints = new List(); + _endpoints.AddRange(endpoints); + _lock = new object(); + + CreateChangeToken(); + } + + public override IChangeToken ChangeToken => _changeToken; + + public override IReadOnlyList Endpoints => _endpoints; + + // Trigger change + public void AddEndpoint(Endpoint endpoint) + { + _endpoints.Add(endpoint); + + // Capture the old tokens so that we can raise the callbacks on them. This is important so that + // consumers do not register callbacks on an inflight event causing a stackoverflow. + var oldTokenSource = _cts; + var oldToken = _changeToken; + + CreateChangeToken(); + + // Raise consumer callbacks. Any new callback registration would happen on the new token + // created in earlier step. + oldTokenSource.Cancel(); + } + + private void CreateChangeToken() + { + _cts = new CancellationTokenSource(); + _changeToken = new CancellationChangeToken(_cts.Token); + } + } +} \ No newline at end of file