Async support for MVC controllers.

[dimarobert]

Closes #4647

Summary

Creates a secondary variant of the MvcHostControl that is executed asynchronously during the page execution.
To avoid any breaking changes to the execution of the existing MVC controls, the control has to opt-in / be marked as requiring asynchronous execution. To do that the control needs to be declared with a ControlSrc in the form of {ControllerNamespace}/{ControllerName}/async/{ActionName}.mvc instead of the {ControllerNamespace}/{ControllerName}/{ActionName}.mvc that existing MVC module controls are defined as.

Apart from the action itself, the retrieval of the ModuleActions (Get{0}Actions method) supports Task<ModuleActionCollection> for asynchronous execution as well.

A few of the methods had to be made protected and as such code had to be re-ordered to comply with StyleCop rules. As a result, it may initially appear that more code than expected was modified.

Other small fixes:

• Fix selection of ControllerName from the segments of the ControlSrc when it has a length greater than 2 when assigning the value for moduleControl.LocalResourceFile.
• MVC: Improve error messages when view template is not found by writing out the SearchedLocations (something that I think ASP.NET does by default).

There is one TODO left in ModuleInstanceContext.cs, and as it states I am not sure if that should return or throw, Throwing, with a good message explaining the issue, would probably help more than the NullReferenceException that would end up being thrown by existing code that would most likely directly iterate over the list received from the method call.

This is also a place where a breaking change could be expected in 3rd party skins or other extensions that try to synchronously read the ModuleInstanceContext.Actions (list of admin ModuleActions) for asynchronous controls.

[bdukes]

Thanks @dimarobert for your efforts and digging into all of the ins-and-outs of the processing going on here!

[mitchelsellers]

@dimarobert thank you so much for this, a few things to review, but great amounts of research.

@donker and @sachatrauwaen I'm curious to see what impacts if any this has on the MVC Pipeline work that is going on as well

[dimarobert]

@dimarobert thank you so much for this, a few things to review, but great amounts of research.

@donker and @sachatrauwaen I'm curious to see what impacts if any this has on the MVC Pipeline work that is going on as well

I will try to give some context for this.

In the WebForms implementation here it was easy enough to hook the async stuff over / in between the existing sync stuff due to the fact that Microsoft provided clear boundaries between the sync and async sections of the page execution through the this.Page.RegisterAsyncTask(new PageAsyncTask(this.MyAsyncMethod)); and also ensures that the async methods that we give to it are safely interleaved with the sync logic of the WebForms page and the transition from sync to async and back to sync does not fall into the async over sync anti-pattern. Execution is also safely done in the correct SynchronizationContext and ExecutionContext so all AsyncLocal variables (like Thread.CurrentCulture, HttpContext.Current, and so on) are available in all stages.

With the MVC implementation I see in #6749 there might be two show stoppers or hard to tackle problems:

1. HtmlHelper does not have support for async in .net framework, like HtmlHelper.PartialAsync() which would be needed to async render child controls. Only ASP.NET Core has HtmlHelper.PartialAsync(). So, if even possible to (did not look into it), this would have to be implemented by DNN. Probably HtmlHelper.ActionAsync() and other helper overloads would have to be provided as an Async overload as well.

2. Everything will have to be async starting at the root. Every place (extension point) where we expect we would need to support async (now or at some point in the future) has to be async, and all callers recursively up the stack have to be async as well. I would say that most of the new abstractions would have to be converted to async-first. Even more important if we add to the discussion that to truly benefit from the async execution we need it to terminate into the different calls to services so threads are freed up while waiting for database calls and (distributed) cache calls for models, file and network I/O calls, and any other operations that are not CPU bound. So as an exercise, to support async module execution we would have to have a Task<IHtmlString> IMvcModuleControl.HtmlAsync(), and everything in the call path has to be async as well:

   • IHtmlString IMvcModuleControl.Html() => Task<IHtmlString> IMvcModuleControl.HtmlAsync()
   • IHtmlString ContainerHelpers.Content() => Task<IHtmlString> ContainerHelpers.ContentAsync()
   • htmlHelper.Partial(container.Value.ContainerRazorFile, container.Value) would be replaced with the DNN implementation of HtmlHelper.PartialAsync()
   • IHtmlString SkinExtensions.Pane() => Task<IHtmlString> SkinExtensions.PaneAsync()

   In this implementation, the interfaces that the system uses should not have any methods for sync execution, only async. For modules that do not desire async, a base implementation of IMvcModuleControl could be provided by DNN that exposes a protected abstract IHtmlString Html() and it implements the async method as a redirect to it Task<IHtmlString> IMvcModuleControl.HtmlAsync() => Task.FromResult(this.Html()).

[sachatrauwaen]

@dimarobert thank you so much for this, a few things to review, but great amounts of research.
@donker and @sachatrauwaen I'm curious to see what impacts if any this has on the MVC Pipeline work that is going on as well

I will try to give some context for this.

In the WebForms implementation here it was easy enough to hook the async stuff over / in between the existing sync stuff due to the fact that Microsoft provided clear boundaries between the sync and async sections of the page execution through the this.Page.RegisterAsyncTask(new PageAsyncTask(this.MyAsyncMethod)); and also ensures that the async methods that we give to it are safely interleaved with the sync logic of the WebForms page and the transition from sync to async and back to sync does not fall into the async over sync anti-pattern. Execution is also safely done in the correct SynchronizationContext and ExecutionContext so all AsyncLocal variables (like Thread.CurrentCulture, HttpContext.Current, and so on) are available in all stages.

With the MVC implementation I see in #6749 there might be two show stoppers or hard to tackle problems:

1. HtmlHelper does not have support for async in .net framework, like HtmlHelper.PartialAsync() which would be needed to async render child controls. Only ASP.NET Core has HtmlHelper.PartialAsync(). So, if even possible to (did not look into it), this would have to be implemented by DNN. Probably HtmlHelper.ActionAsync() and other helper overloads would have to be provided as an Async overload as well.

2. Everything will have to be async starting at the root. Every place (extension point) where we expect we would need to support async (now or at some point in the future) has to be async, and all callers recursively up the stack have to be async as well. I would say that most of the new abstractions would have to be converted to async-first. Even more important if we add to the discussion that to truly benefit from the async execution we need it to terminate into the different calls to services so threads are freed up while waiting for database calls and (distributed) cache calls for models, file and network I/O calls, and any other operations that are not CPU bound. So as an exercise, to support async module execution we would have to have a Task<IHtmlString> IMvcModuleControl.HtmlAsync(), and everything in the call path has to be async as well:

   • IHtmlString IMvcModuleControl.Html() => Task<IHtmlString> IMvcModuleControl.HtmlAsync()
   • IHtmlString ContainerHelpers.Content() => Task<IHtmlString> ContainerHelpers.ContentAsync()
   • htmlHelper.Partial(container.Value.ContainerRazorFile, container.Value) would be replaced with the DNN implementation of HtmlHelper.PartialAsync()
   • IHtmlString SkinExtensions.Pane() => Task<IHtmlString> SkinExtensions.PaneAsync()

   In this implementation, the interfaces that the system uses should not have any methods for sync execution, only async. For modules that do not desire async, a base implementation of IMvcModuleControl could be provided by DNN that exposes a protected abstract IHtmlString Html() and it implements the async method as a redirect to it Task<IHtmlString> IMvcModuleControl.HtmlAsync() => Task.FromResult(this.Html()).

Great !
For the mvc pipeline, i think it is not a problem not to have HtmlHelper.PartialAsync as long as you do not put logic in your partials (and html helpers used in your partials).

But Html helpers on witch a lot is based in the mvc pipeline are synchronous by design in MVC 5.
Remark : In .net framework you need to be very carryfull to avoid the Sync Context Deadlock problem.
The solution is dotnet core ViewComponent !
The mvc 5 controllers can use async and then put all you async stuff here, and build the whole model here.
But actually in the mvc pipeline, it is not this way. Modules are rendererd more la ViewComponent and are independent of the controller.

[dimarobert]

But actually in the mvc pipeline, it is not this way. Modules are rendererd more la ViewComponent and are independent of the controller.

So, if Modules in the MvcPipeline do not have access to the controller and the only API surface that the module developer has access to is the IHtmlString IMvcModuleControl.Html(), doesn't that mean that MvcPipeline will be unable to support async execution for its modules? (unless the approach I gave above is implemented - with everything being async top to bottom in the MvcPipeline execution - which I see as a considerable API change to be made after the fact)

[sachatrauwaen]

But actually in the mvc pipeline, it is not this way. Modules are rendererd more la ViewComponent and are independent of the controller.

So, if Modules in the MvcPipeline do not have access to the controller and the only API surface that the module developer has access to is the IHtmlString IMvcModuleControl.Html(), doesn't that mean that MvcPipeline will be unable to support async execution for its modules? (unless the approach I gave above is implemented - with everything being async top to bottom in the MvcPipeline execution - which I see as a considerable API change to be made after the fact)

The modules can have access to the controller, but it run all in a unic controller.
I think you are right, MvcPipeline will be unable to support async execution for its modules (because existing reusable components in mvc 5, partial and child controllers, dous not support async).
If i understand it well your approach is based on PageAsyncTask witch is a pure webform stuff, so not appicable to the mvc pipeline.
I read, "Razor views in MVC 5 are rendered by a synchronous rendering engine. The Razor view engine does not understand await".

That's a raison why we want DNN.Core :)

[dimarobert]

As I said in my initial comment on this "... if even possible to implement such a HtmlHelper.PartialAsync() (did not look into it)". Now I looked more into it and looks close to impossible to do. The entire stack is sync, event the runtime generated class of the cshtml writes its output through a sync Execute() method that gets called from WebPageBase.ExecutePageHierarchy().

There is however the HtmlHelper.Action() helper that I would expect to be able to somehow run for any controller action, even if it is async, so they are hacking it in somehow and waiting for it - similar to WebForms async point. But even if we figure out how they do it for Actions, the big hurdle is that sync Execute() method that gets generated by the compilation of the cshtml view. I do not see how that could be overcome.

So, looks quite unlikely for the MvcPipeline Razor+ Modules (I hope that is the correct terminology) to be able to support async.

[sachatrauwaen]

As I said in my initial comment on this "... if even possible to implement such a HtmlHelper.PartialAsync() (did not look into it)". Now I looked more into it and looks close to impossible to do. The entire stack is sync, event the runtime generated class of the cshtml writes its output through a sync Execute() method that gets called from WebPageBase.ExecutePageHierarchy().

There is however the HtmlHelper.Action() helper that I would expect to be able to somehow run for any controller action, even if it is async, so they are hacking it in somehow and waiting for it - similar to WebForms async point. But even if we figure out how they do it for Actions, the big hurdle is that sync Execute() method that gets generated by the compilation of the cshtml view. I do not see how that could be overcome.

So, looks quite unlikely for the MvcPipeline Razor+ Modules (I hope that is the correct terminology) to be able to support async.

HtmlHelper.Action is for executing Child Action Controllers, and from what i read, they can not run async. And because there called from a html helper from a razor view, it confirm that they cannot run async. And because Child Controllers are not supported in dotnet core, i prefer to stay on partials and/or ViewComponent patern.