Add dynamic controller/page routes #8955

rynowak · 2019-03-31T21:50:56Z

Adds infrastructure for a common IRouter-based pattern. In this pattern,
an extender subclasses Route to post-process the route values before MVC
action selection run. The new infrastructure duplicates this kind of
experience but based on endpoint routing.

The approach in this PR starts at the bottom... meaning that this is the
most-focused and least-invasive way to implement a feature like this.
Similar to fallback routing, this is a pattern built with matcher
policies and metadata rather than a built-in feature of routing.

It's valuable to point out that this approach uses IActionConstraint to
disambiguate between actions. The other way we could go would be to make
the other matcher policy implementations able to do this. This would
mean that whenever you have a dynamic endpoint, you will not by using
the DFA for features like HTTP methods. It also means that we need to go
re-implement a bunch of infrastructure.

This PR also adds the concept of an 'inert' endpoint - a non-Routable
endpoint that's created when fallback/dynamic is in use. This seems like
a cleaner design because we don't start matching RouteEndpoint
instances for URLs that don't match. This resolves #8130

src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs

src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs

src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs

src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs

src/Mvc/Mvc.Core/src/Routing/DynamicRouteValueTransformer.cs

src/Mvc/Mvc.Core/test/Infrastructure/ActionSelectionTableTest.cs

src/Mvc/test/WebSites/RoutingWebSite/Areas/Admin/DynamicController.cs

rynowak · 2019-06-20T13:40:08Z

@JamesNK @pranavkm - can you please take a look? This is for preview 7.

davidfowl · 2019-06-20T14:07:54Z

src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs

+    public abstract partial class DynamicRouteValueTransformer
+    {
+        protected DynamicRouteValueTransformer() { }
+        public abstract System.Threading.Tasks.Task<Microsoft.AspNetCore.Routing.RouteValueDictionary> TransformAsync(Microsoft.AspNetCore.Http.HttpContext httpContext, Microsoft.AspNetCore.Routing.RouteValueDictionary values);


Why? Is the guidance to just use ValueTask everywhere now?

I am aware that you most probably know this, and your question is partially rhetoric, but..
I think the guidance (as of "Stephen Toub says so") is that if the method is mostly expected to return synchronously (like in a "streaming over bytes that are most of the time in memory but rarely have to be fetched over the wire" scenario), use a ValueTask, but if the method is expected to do real async work reasonably often (like let's say > 20% of the time, I'm not sure where I got this number from, but it stuck) use Task, as ValueTask comes with it's own struct-based overhead and just wraps a real Task in that case..

ValueTask<T> is still the guidance, there's almost no reason to use Task<T> as it forces an allocation. You can also now reuse allocations with ValueTask<T> so it's an extensibility point as well.

Why? Is the guidance to just use ValueTask everywhere now?

Yes, generic ValueTask<T> absolutely, for ValueTask is more questionable and has more overhead and is only really valuable as an extensibility point (using IValueTaskSource)

hrm didn't sound like it here: https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/#user-content-should-every-new-asynchronous-api-return-valuetask--valuetasktresult
"In short, no: the default choice is still Task / Task."
...
" There are also some minor costs associated with returning a ValueTask instead of a Task, e.g. in microbenchmarks it’s a bit faster to await a Task than it is to await a ValueTask, so if you can use cached tasks (e.g. you’re API returns Task or Task), you might be better off performance-wise sticking with Task and Task. ValueTask / ValueTask are also multiple words in size, and so when these are awaitd and a field for them is stored in a calling async method’s state machine, they’ll take up a little more space in that state machine object."

Honstely that#s a bit over my head though... back then I read "use ValueTask only when it completes mosty synchronous" out of it

However, ValueTask / ValueTask are great choices when a) you expect consumers of your API to only await them directly, b) allocation-related overhead is important to avoid for your API, and c) either you expect synchronous completion to be a very common case, or you’re able to effectively pool objects for use with asynchronous completion. When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

If you have a virtual member, you can't assume what derived implementations will do. To optimize for common synchronous cases and offer extensibility for derived types, use ValueTask<T>.

The framework is the one that is awaiting this so it's mostly up to the implementation to just return synchronously or asynchronously. The other downsides don't apply.

I really want to go back the change a bunch of APIs to return ValueTask<T> 😄 . I could squash so many needless allocations in the auth stack..

Appreciate the time you took to answer that. Hopefully someone finds this hidden conversation, reads it and learns, too.

rynowak · 2019-06-21T20:07:07Z

@JamesNK @pranavkm - please take a look

src/Mvc/Mvc.RazorPages/src/Builder/RazorPagesEndpointRouteBuilderExtensions.cs

pranavkm

👍

src/Mvc/Mvc.Core/src/Builder/ControllerEndpointRouteBuilderExtensions.cs

src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs

src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs

src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs

src/Mvc/Mvc.Core/src/Routing/DynamicRouteValueTransformer.cs

src/Mvc/Mvc.RazorPages/src/Infrastructure/PageActionEndpointDataSource.cs

src/Mvc/test/WebSites/RoutingWebSite/StartupForDynamic.cs

Adds infrastructure for a common IRouter-based pattern. In this pattern, an extender subclasses Route to post-process the route values before MVC action selection run. The new infrastructure duplicates this kind of experience but based on endpoint routing. The approach in this PR starts at the bottom... meaning that this is the most-focused and least-invasive way to implement a feature like this. Similar to fallback routing, this is a pattern built with matcher policies and metadata rather than a built-in feature of routing. It's valuable to point out that this approach uses IActionConstraint to disambiguate between actions. The other way we could go would be to make the *other* matcher policy implementations able to do this. This would mean that whenever you have a dynamic endpoint, you will not by using the DFA for features like HTTP methods. It also means that we need to go re-implement a bunch of infrastructure. This PR also adds the concept of an 'inert' endpoint - a non-Routable endpoint that's created when fallback/dynamic is in use. This seems like a cleaner design because we don't start *matching* RouteEndpoint instances for URLs that don't match. This resolves #8130

rynowak requested review from davidfowl, pranavkm and JamesNK March 31, 2019 21:50

rynowak added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 31, 2019