Invalidate PageActionInvokerProvider cache when compilation cache expires #6438
Conversation
|
|
||
| IFilterMetadata[] filters; | ||
| if (!cache.Entries.TryGetValue(actionDescriptor, out cacheEntry)) | ||
| if (!cache.Entries.TryGetValue(actionDescriptor, out PageActionInvokerCacheEntry cacheEntry)) |
There was a problem hiding this comment.
The action descriptor(s) would still be valid if the compilation expires, but it's just the cached types that need to be invalidated. Thoughts \ concerns?
|
|
||
| namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure | ||
| { | ||
| public struct PageLoaderResult |
There was a problem hiding this comment.
I really don't like this approach. That's how we ended up with spaghetti in the view engine.
There was a problem hiding this comment.
Should this just trigger a full rediscovery of all pages? What do you think is better?
I think the code you wrote is reasonable, but I don't like the approach of having two levels of caching. that can both get invalidated.
There was a problem hiding this comment.
We do have a couple of options here:
a) Pump the compilation change tokens to an IActionDescriptorChangeProvider. This seems difficult though given the layering.
b) We could add change tokens _ViewImports for all files as part of the PageActionDescriptorChangeProvider. It's not super bad since already do file traversals as part of the PageADP. Given that there's so there's so much overlap in the actual number of distinct change tokens, this seems a bit unnecessary.
c) We could add change tokens for _ViewImports for a (non-existent) file at Pages root. All the other _ViewImports are already tracked by the **/*.cshtml.
Thoughts on the lot?
There was a problem hiding this comment.
Can you explain c to me? Why isn't Pages/_ViewImports.cshtml included by **/*.cshtml?
There was a problem hiding this comment.
I'd really prefer something like c.
There was a problem hiding this comment.
The issue here is the /_ViewImports.cshtml (the one above the /Pages directory. For (c) we say GetHierarchicalItems('${PagesRoot}/Index.cshtml") and watch all those files along with $"{PagesRoot}/**/*.cshtml"
|
|
||
| IFilterMetadata[] filters; | ||
| if (!cache.Entries.TryGetValue(actionDescriptor, out cacheEntry)) | ||
| if (!cache.Entries.TryGetValue(actionDescriptor, out PageActionInvokerCacheEntry cacheEntry)) |
| @@ -43,6 +43,11 @@ public class ActionDescriptorCollectionProvider : IActionDescriptorCollectionPro | |||
|
|
|||
| private IChangeToken GetCompositeChangeToken() | |||
| { | |||
| if (_actionDescriptorChangeProviders.Length == 1) | |||
There was a problem hiding this comment.
Just seemed sensible not to have nested CompositeChangeTokens in the most common case.
| .Select(item => item.Path) | ||
| .ToArray(); | ||
|
|
||
| _searchPattern = rootDirectory + "/**/*.cshtml"; |
There was a problem hiding this comment.
Can you make this breathe a bit?
Fixes #6428