Skip to content

Commit 7fee202

Browse files
pranavkmpranavkm
committed
Changes per PR comments
* Use ActionDescriptorCollection.Version to invalidate PageLoader cache * Add tests, changes per PR
1 parent 79e6d0e commit 7fee202

File tree

9 files changed

+245
-86
lines changed

9 files changed

+245
-86
lines changed

‎src/Mvc/Mvc.RazorPages/src/DependencyInjection/MvcRazorPagesMvcCoreBuilderExtensions.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,9 @@ internal static void AddServices(IServiceCollection services)
117117
services.TryAddSingleton<IPageFactoryProvider, DefaultPageFactoryProvider>();
118118

119119
#pragma warning disable CS0618 // Type or member is obsolete
120-
services.TryAddSingleton<IPageLoader>(s => s.GetRequiredService<PageLoaderBase>());
120+
services.TryAddSingleton<IPageLoader>(s => s.GetRequiredService<PageLoader>());
121121
#pragma warning restore CS0618 // Type or member is obsolete
122-
services.TryAddSingleton<PageLoaderBase, DefaultPageLoader>();
122+
services.TryAddSingleton<PageLoader, DefaultPageLoader>();
123123
services.TryAddSingleton<IPageHandlerMethodSelector, DefaultPageHandlerMethodSelector>();
124124

125125
// Action executors

‎src/Mvc/Mvc.RazorPages/src/Infrastructure/DefaultPageLoader.cs

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.Linq;
78
using System.Reflection;
@@ -10,35 +11,36 @@
1011
using Microsoft.AspNetCore.Http;
1112
using Microsoft.AspNetCore.Mvc.ApplicationModels;
1213
using Microsoft.AspNetCore.Mvc.Filters;
14+
using Microsoft.AspNetCore.Mvc.Infrastructure;
1315
using Microsoft.AspNetCore.Mvc.Razor.Compilation;
1416
using Microsoft.AspNetCore.Mvc.Routing;
15-
using Microsoft.Extensions.Caching.Memory;
1617
using Microsoft.Extensions.Options;
1718

1819
namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
1920
{
20-
internal class DefaultPageLoader : PageLoaderBase
21+
internal class DefaultPageLoader : PageLoader
2122
{
23+
private readonly IActionDescriptorCollectionProvider _collectionProvider;
2224
private readonly IPageApplicationModelProvider[] _applicationModelProviders;
2325
private readonly IViewCompilerProvider _viewCompilerProvider;
2426
private readonly ActionEndpointFactory _endpointFactory;
2527
private readonly PageConventionCollection _conventions;
2628
private readonly FilterCollection _globalFilters;
27-
private readonly MemoryCache _memoryCache;
29+
private volatile InnerCache _currentCache;
2830

2931
public DefaultPageLoader(
32+
IActionDescriptorCollectionProvider actionDescriptorCollectionProvider,
3033
IEnumerable<IPageApplicationModelProvider> applicationModelProviders,
3134
IViewCompilerProvider viewCompilerProvider,
3235
ActionEndpointFactory endpointFactory,
3336
IOptions<RazorPagesOptions> pageOptions,
3437
IOptions<MvcOptions> mvcOptions)
3538
{
39+
_collectionProvider = actionDescriptorCollectionProvider;
3640
_applicationModelProviders = applicationModelProviders
3741
.OrderBy(p => p.Order)
3842
.ToArray();
3943

40-
_memoryCache = new MemoryCache(new MemoryCacheOptions());
41-
4244
_viewCompilerProvider = viewCompilerProvider;
4345
_endpointFactory = endpointFactory;
4446
_conventions = pageOptions.Value.Conventions;
@@ -47,31 +49,42 @@ public DefaultPageLoader(
4749

4850
private IViewCompiler Compiler => _viewCompilerProvider.GetCompiler();
4951

50-
public async override ValueTask<CompiledPageActionDescriptor> LoadAsync(PageActionDescriptor actionDescriptor)
52+
private ConcurrentDictionary<PageActionDescriptor, Task<CompiledPageActionDescriptor>> CurrentCache
5153
{
52-
if (actionDescriptor == null)
54+
get
5355
{
54-
throw new ArgumentNullException(nameof(actionDescriptor));
56+
var current = _currentCache;
57+
var actionDescriptors = _collectionProvider.ActionDescriptors;
58+
59+
if (current == null || current.Version != actionDescriptors.Version)
60+
{
61+
current = new InnerCache(actionDescriptors.Version);
62+
_currentCache = current;
63+
}
64+
65+
return current.Entries;
5566
}
67+
}
5668

57-
if (_memoryCache.TryGetValue(actionDescriptor, out CompiledPageActionDescriptor compiledDescriptor))
69+
public override Task<CompiledPageActionDescriptor> LoadAsync(PageActionDescriptor actionDescriptor)
70+
{
71+
if (actionDescriptor == null)
5872
{
59-
return compiledDescriptor;
73+
throw new ArgumentNullException(nameof(actionDescriptor));
6074
}
6175

62-
var viewDescriptor = await Compiler.CompileAsync(actionDescriptor.RelativePath);
63-
var compiledPageActionDescriptor = GetCompiledPageActionDescriptor(actionDescriptor, viewDescriptor);
64-
var entryOptions = new MemoryCacheEntryOptions();
65-
for (var i = 0; i < viewDescriptor.ExpirationTokens.Count; i++)
76+
var cache = CurrentCache;
77+
if (cache.TryGetValue(actionDescriptor, out var compiledDescriptorTask))
6678
{
67-
entryOptions.ExpirationTokens.Add(viewDescriptor.ExpirationTokens[i]);
79+
return compiledDescriptorTask;
6880
}
6981

70-
return _memoryCache.Set(actionDescriptor, compiledPageActionDescriptor, entryOptions);
82+
return cache.GetOrAdd(actionDescriptor, LoadAsyncCore(actionDescriptor));
7183
}
7284

73-
private CompiledPageActionDescriptor GetCompiledPageActionDescriptor(PageActionDescriptor actionDescriptor, CompiledViewDescriptor viewDescriptor)
85+
private async Task<CompiledPageActionDescriptor> LoadAsyncCore(PageActionDescriptor actionDescriptor)
7486
{
87+
var viewDescriptor = await Compiler.CompileAsync(actionDescriptor.RelativePath);
7588
var context = new PageApplicationModelProviderContext(actionDescriptor, viewDescriptor.Type.GetTypeInfo());
7689
for (var i = 0; i < _applicationModelProviders.Length; i++)
7790
{
@@ -149,5 +162,18 @@ IEnumerable<TConvention> GetConventions<TConvention>(
149162
attributes.OfType<TConvention>());
150163
}
151164
}
165+
166+
private sealed class InnerCache
167+
{
168+
public InnerCache(int version)
169+
{
170+
Version = version;
171+
Entries = new ConcurrentDictionary<PageActionDescriptor, Task<CompiledPageActionDescriptor>>();
172+
}
173+
174+
public ConcurrentDictionary<PageActionDescriptor, Task<CompiledPageActionDescriptor>> Entries { get; }
175+
176+
public int Version { get; }
177+
}
152178
}
153179
}

‎src/Mvc/Mvc.RazorPages/src/Infrastructure/IPageLoader.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
88
/// <summary>
99
/// Creates a <see cref="CompiledPageActionDescriptor"/> from a <see cref="PageActionDescriptor"/>.
1010
/// </summary>
11-
[Obsolete("This type is obsolete. Use PageLoaderBase instead.")]
11+
[Obsolete("This type is obsolete. Use " + nameof(PageLoader) + " instead.")]
1212
public interface IPageLoader
1313
{
1414
/// <summary>

‎src/Mvc/Mvc.RazorPages/src/Infrastructure/PageActionInvokerProvider.cs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
2020
{
2121
internal class PageActionInvokerProvider : IActionInvokerProvider
2222
{
23-
private readonly PageLoaderBase _loader;
23+
private readonly PageLoader _loader;
2424
private readonly IPageFactoryProvider _pageFactoryProvider;
2525
private readonly IPageModelFactoryProvider _modelFactoryProvider;
2626
private readonly IModelBinderFactory _modelBinderFactory;
@@ -42,7 +42,7 @@ internal class PageActionInvokerProvider : IActionInvokerProvider
4242
private volatile InnerCache _currentCache;
4343

4444
public PageActionInvokerProvider(
45-
PageLoaderBase loader,
45+
PageLoader loader,
4646
IPageFactoryProvider pageFactoryProvider,
4747
IPageModelFactoryProvider modelFactoryProvider,
4848
IRazorPageFactoryProvider razorPageFactoryProvider,
@@ -80,7 +80,7 @@ public PageActionInvokerProvider(
8080
}
8181

8282
public PageActionInvokerProvider(
83-
PageLoaderBase loader,
83+
PageLoader loader,
8484
IPageFactoryProvider pageFactoryProvider,
8585
IPageModelFactoryProvider modelFactoryProvider,
8686
IRazorPageFactoryProvider razorPageFactoryProvider,
@@ -139,14 +139,15 @@ public void OnProvidersExecuting(ActionInvokerProviderContext context)
139139
if (!cache.Entries.TryGetValue(actionDescriptor, out var cacheEntry))
140140
{
141141
CompiledPageActionDescriptor compiledPageActionDescriptor;
142-
var endpointFeature = actionContext.HttpContext.Features.Get<IEndpointFeature>();
143-
if (endpointFeature != null)
142+
if (_mvcOptions.EnableEndpointRouting)
144143
{
145144
// With endpoint routing, PageLoaderMatcherPolicy should have already produced a CompiledPageActionDescriptor.
146145
compiledPageActionDescriptor = (CompiledPageActionDescriptor)actionDescriptor;
147146
}
148147
else
149148
{
149+
// With legacy routing, we're forced to perform a blocking call. The exceptation is that
150+
// in the most common case - build time views or successsively cached runtime views - this should finish synchronously.
150151
compiledPageActionDescriptor = _loader.LoadAsync(actionDescriptor).GetAwaiter().GetResult();
151152
}
152153

‎src/Mvc/Mvc.RazorPages/src/Infrastructure/PageLoaderBase.cs renamed to ‎src/Mvc/Mvc.RazorPages/src/Infrastructure/PageLoader.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
99
/// Creates a <see cref="CompiledPageActionDescriptor"/> from a <see cref="PageActionDescriptor"/>.
1010
/// </summary>
1111
#pragma warning disable CS0618 // Type or member is obsolete
12-
public abstract class PageLoaderBase : IPageLoader
12+
public abstract class PageLoader : IPageLoader
1313
#pragma warning restore CS0618 // Type or member is obsolete
1414
{
15-
public abstract ValueTask<CompiledPageActionDescriptor> LoadAsync(PageActionDescriptor actionDescriptor);
15+
public abstract Task<CompiledPageActionDescriptor> LoadAsync(PageActionDescriptor actionDescriptor);
1616

1717
CompiledPageActionDescriptor IPageLoader.Load(PageActionDescriptor actionDescriptor)
1818
=> LoadAsync(actionDescriptor).GetAwaiter().GetResult();

‎src/Mvc/Mvc.RazorPages/src/Infrastructure/PageLoaderMatcherPolicy.cs

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
1212
{
1313
internal class PageLoaderMatcherPolicy : MatcherPolicy, IEndpointSelectorPolicy
1414
{
15-
private readonly PageLoaderBase _loader;
15+
private readonly PageLoader _loader;
1616

17-
public PageLoaderMatcherPolicy(PageLoaderBase loader)
17+
public PageLoaderMatcherPolicy(PageLoader loader)
1818
{
1919
if (loader == null)
2020
{
@@ -68,7 +68,7 @@ public Task ApplyAsync(HttpContext httpContext, EndpointSelectorContext context,
6868
{
6969
throw new ArgumentNullException(nameof(candidates));
7070
}
71-
71+
7272
for (var i = 0; i < candidates.Count; i++)
7373
{
7474
ref var candidate = ref candidates[i];
@@ -86,27 +86,30 @@ public Task ApplyAsync(HttpContext httpContext, EndpointSelectorContext context,
8686
}
8787
else
8888
{
89-
// In the most common case, LoadAsync will return a synchronous result.
89+
// In the most common case, GetOrAddAsync will return a synchronous result.
9090
// Avoid going async since this is a fairly hot path.
91-
return ApplyAsyncAwaited(candidates);
91+
return ApplyAsyncAwaited(candidates, compiled, i);
9292
}
9393
}
9494
}
9595

9696
return Task.CompletedTask;
9797
}
9898

99-
private async Task ApplyAsyncAwaited(CandidateSet candidates)
99+
private async Task ApplyAsyncAwaited(CandidateSet candidates, Task<CompiledPageActionDescriptor> actionDescriptorTask, int index)
100100
{
101-
for (var i = 0; i < candidates.Count; i++)
101+
var compiled = await actionDescriptorTask;
102+
candidates.ReplaceEndpoint(index, compiled.Endpoint, candidates[index].Values);
103+
104+
for (var i = index + 1; i < candidates.Count; i++)
102105
{
103106
var candidate = candidates[i];
104107
var endpoint = candidate.Endpoint;
105108

106109
var page = endpoint.Metadata.GetMetadata<PageActionDescriptor>();
107110
if (page != null)
108111
{
109-
var compiled = await _loader.LoadAsync(page);
112+
compiled = await _loader.LoadAsync(page);
110113
candidates.ReplaceEndpoint(i, compiled.Endpoint, candidates[i].Values);
111114
}
112115
}

‎src/Mvc/Mvc.RazorPages/test/Infrastructure/DefaultPageLoaderTest.cs

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
using System;
55
using System.Reflection;
66
using System.Threading.Tasks;
7+
using Microsoft.AspNetCore.Mvc.Abstractions;
78
using Microsoft.AspNetCore.Mvc.ApplicationModels;
9+
using Microsoft.AspNetCore.Mvc.Infrastructure;
810
using Microsoft.AspNetCore.Mvc.Razor.Compilation;
911
using Microsoft.AspNetCore.Mvc.Routing;
1012
using Microsoft.AspNetCore.Razor.Hosting;
@@ -18,6 +20,14 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
1820
{
1921
public class DefaultPageLoaderTest
2022
{
23+
private readonly IActionDescriptorCollectionProvider ActionDescriptorCollectionProvider;
24+
25+
public DefaultPageLoaderTest()
26+
{
27+
var actionDescriptors = new ActionDescriptorCollection(Array.Empty<ActionDescriptor>(), 1);
28+
ActionDescriptorCollectionProvider = Mock.Of<IActionDescriptorCollectionProvider>(v => v.ActionDescriptors == actionDescriptors);
29+
}
30+
2131
[Fact]
2232
public async Task LoadAsync_InvokesApplicationModelProviders()
2333
{
@@ -77,6 +87,7 @@ public async Task LoadAsync_InvokesApplicationModelProviders()
7787
};
7888

7989
var loader = new DefaultPageLoader(
90+
ActionDescriptorCollectionProvider,
8091
providers,
8192
compilerProvider,
8293
endpointFactory,
@@ -132,6 +143,7 @@ public async Task LoadAsync_CreatesEndpoint_WithRoute()
132143
};
133144

134145
var loader = new DefaultPageLoader(
146+
ActionDescriptorCollectionProvider,
135147
providers,
136148
compilerProvider,
137149
endpointFactory,
@@ -196,6 +208,7 @@ public async Task LoadAsync_InvokesApplicationModelProviders_WithTheRightOrder()
196208
};
197209

198210
var loader = new DefaultPageLoader(
211+
ActionDescriptorCollectionProvider,
199212
providers,
200213
compilerProvider,
201214
endpointFactory,
@@ -251,6 +264,7 @@ public async Task LoadAsync_CachesResults()
251264
};
252265

253266
var loader = new DefaultPageLoader(
267+
ActionDescriptorCollectionProvider,
254268
providers,
255269
compilerProvider,
256270
endpointFactory,
@@ -265,6 +279,71 @@ public async Task LoadAsync_CachesResults()
265279
Assert.Same(result1, result2);
266280
}
267281

282+
[Fact]
283+
public async Task LoadAsync_UpdatesResults()
284+
{
285+
// Arrange
286+
var descriptor = new PageActionDescriptor()
287+
{
288+
AttributeRouteInfo = new AttributeRouteInfo()
289+
{
290+
Template = "/test",
291+
},
292+
};
293+
294+
var transformer = new Mock<RoutePatternTransformer>();
295+
transformer
296+
.Setup(t => t.SubstituteRequiredValues(It.IsAny<RoutePattern>(), It.IsAny<object>()))
297+
.Returns<RoutePattern, object>((p, v) => p);
298+
299+
var compilerProvider = GetCompilerProvider();
300+
301+
var razorPagesOptions = Options.Create(new RazorPagesOptions());
302+
var mvcOptions = Options.Create(new MvcOptions());
303+
var endpointFactory = new ActionEndpointFactory(transformer.Object);
304+
305+
var provider = new Mock<IPageApplicationModelProvider>();
306+
307+
var pageApplicationModel = new PageApplicationModel(descriptor, typeof(object).GetTypeInfo(), Array.Empty<object>());
308+
309+
provider.Setup(p => p.OnProvidersExecuting(It.IsAny<PageApplicationModelProviderContext>()))
310+
.Callback((PageApplicationModelProviderContext c) =>
311+
{
312+
Assert.Null(c.PageApplicationModel);
313+
c.PageApplicationModel = pageApplicationModel;
314+
})
315+
.Verifiable();
316+
317+
var providers = new[]
318+
{
319+
provider.Object,
320+
};
321+
322+
var descriptorCollection1 = new ActionDescriptorCollection(new[] { descriptor }, version: 1);
323+
var descriptorCollection2 = new ActionDescriptorCollection(new[] { descriptor }, version: 2);
324+
325+
var actionDescriptorCollectionProvider = new Mock<IActionDescriptorCollectionProvider>();
326+
actionDescriptorCollectionProvider
327+
.SetupSequence(p => p.ActionDescriptors)
328+
.Returns(descriptorCollection1)
329+
.Returns(descriptorCollection2);
330+
331+
var loader = new DefaultPageLoader(
332+
actionDescriptorCollectionProvider.Object,
333+
providers,
334+
compilerProvider,
335+
endpointFactory,
336+
razorPagesOptions,
337+
mvcOptions);
338+
339+
// Act
340+
var result1 = await loader.LoadAsync(descriptor);
341+
var result2 = await loader.LoadAsync(descriptor);
342+
343+
// Assert
344+
Assert.NotSame(result1, result2);
345+
}
346+
268347
[Fact]
269348
public void ApplyConventions_InvokesApplicationModelConventions()
270349
{

0 commit comments

Comments
 (0)