U4-7270 - Async Index actions for controllers implementing IRenderMvcController do not render view correctly

Created by Kieron McIntyre 17 Oct 2015, 13:54:36 Updated by Claus Jensen 24 Nov 2015, 10:41:13

Relates to: U4-7399

When I create an async Index action on a custom controller, it renders System.Threading.Tasks.Task'1[System.Web.Mvc.ActionResult] instead of the expected published page.

public async Task<ActionResult> Index(RenderModel model, int? page, string query){ return CurrentTemplate(model);}

This is related to an issue fixed here (http://issues.umbraco.org/issue/U4-5208). However, if I create an async action that is routed to the template's name, the published page is rendered correctly.

public async Task<ActionResult> NewsListing(RenderModel model, int? page, string query){ return CurrentTemplate(model);}

I've written about it here: http://www.digbyswift.com/blog/2015/10/asyncawait-in-umbraco-custom-controllers/ if you want a breakdown. It seems to be linked to the invocation of any Index() action but specifically on a controller that implements IRenderMvcController. This is because the rendering of async actions works fine in controllers that inherit from SurrfaceController and they have no reliance on IRenderMvcController. However in the hybrid framework, the controllers all inherit from SurfaceController but also implement IRenderMvcController and the issue arises here too.

Comments

Kieron McIntyre 23 Oct 2015, 09:15:01

I've figured out what is causing the issue. The code that extracts the action descriptor for the matched Index action is treating all descriptors as synchronous (i.e. ReflectedActionDescriptor) however, an async action will generate a TaskAsyncActionDescriptor. All the descriptors are also then being cached in the concurrent dictionary as synchronous descriptors.


Kieron McIntyre 23 Oct 2015, 09:15:27

I've now added a patch for this here: https://github.com/umbraco/Umbraco-CMS/pull/837


Shannon Deminick 28 Oct 2015, 13:34:42

I have greatly simplified the process in this commit: ecbced5c62fcb9853f68575a5e28f026b7290068

Before we were using our own reflection to determine the action based on a method info and caching it. This is less than ideal considering that MVC already does all of this for us... however I was not aware of the methods.

All of that can simply be changed to:

return controllerDescriptor.FindAction(controllerContext, "Index")

Which will go use MVC to lookup the Index action given the current controller. This also automatically checks for the NonAction and ActionName attributes for us and does the caching. Furthermore, this also returns the correct ActionDescriptor to support either async or non async.

I have tested this with:

  • Default Umbraco install
  • Creating a hijacked route like this:
    public class TestController : RenderMvcController
    {
        [NonAction]
        public override ActionResult Index(RenderModel model)
        {
            return CurrentTemplate(model);
        }

        [ActionName("Index")]
        public async Task<ActionResult> Hello(RenderModel model)
        {
            return await Task.FromResult<ActionResult>(Content("Hello!!"));         
        }
    }

And this successfully executes the async Index method correctly.


Kieron McIntyre 28 Oct 2015, 14:40:43

Awesome. Yes, I was wondering if that would work but I didn't want to commit something so drastic. Glad we got a tidy solution out of it.


Kieron McIntyre 02 Nov 2015, 09:43:00

I've gone back to my test project and I'm not convinced this is the correct approach. Although cleaner, the latest commit doesn't actually solve the issue.

You're suggesting that in order to have an async action first I have to override the base.Index() and mark it as a [NonAction] and then I can have another action that is async but marked with [ActionName("Index")]. This is redundant since I can just name the async action the same as the template and this will take precedence over the Index action anyway, and this also already worked with the previous RenderActionInvoker class. Alternatively, I could add a new keyword to the Index action, although I dislike that option.

The issue here is that we currently cannot have an async Index() action. Obviously the only way to achieve this is to overload the Index action so that it has a different parameter signature, e.g. Task<ActionResult> Index(RenderModel model, int? page) but then the new RenderActionInvoker revision you've committed won't work with this as it throws an AmbiguousMatchException. However, the code I committed however ''will'' work with this scenario although granted it needs tweaking to cater for the caching of the action descriptors.

If you want I'll work up a patch that corrects my code and uses the ConcurrentDictionary correctly.


Kieron McIntyre 02 Nov 2015, 09:45:37

Actually, here it is. Let me know if you want this committing:

{code}	public class CustomActionInvoker : AsyncControllerActionInvoker
{

	private static readonly ConcurrentDictionary<Type, ActionDescriptor> IndexDescriptors = new ConcurrentDictionary<Type, ActionDescriptor>(); 

	protected override ActionDescriptor FindAction(ControllerContext controllerContext, ControllerDescriptor controllerDescriptor, string actionName)
	{
		var ad = base.FindAction(controllerContext, controllerDescriptor, actionName);

		if (ad == null)
		{
			if (controllerContext.Controller is IRenderMvcController)
			{
				return IndexDescriptors.GetOrAdd(controllerContext.Controller.GetType(), type =>
				{
					var methodInfo = controllerContext.Controller.GetType().GetMethods()
						.First(x => x.Name == "Index" &&
									x.GetCustomAttributes(typeof (NonActionAttribute), false).Any() == false);

					return typeof (Task).IsAssignableFrom(methodInfo.ReturnType)
						? new TaskAsyncActionDescriptor(methodInfo, "Index", controllerDescriptor)
						: (ActionDescriptor) new ReflectedActionDescriptor(methodInfo, "Index", controllerDescriptor);
				});
			}
		}

		return ad;
	}

}


Kieron McIntyre 02 Nov 2015, 10:24:00

I've submitted a pull request now.


Shannon Deminick 02 Nov 2015, 17:30:43

Hi, yes this is all true, you cannot have an async Index action with the same signature... because that would be a breaking change to IRenderMvcController. You have listed the alternative options:

  • using the 'new' keyword might work
  • using a different Index signature will work

Of course you can work around this limitation by using your custom action invoker you've created above and applying that to your controller.

As for the Umbraco core changes, I'd rather not having this custom Action Invoker and instead use the native .Net code so we don't have to re-invent the wheel. The problem that we're actually trying to solve is how to have an async Index action for IRenderMvcController. So I think a more interesting solution might be to create an IAsyncRenderMvcController with async Index action definition and integrate that accordingly.


Kieron McIntyre 02 Nov 2015, 17:44:16

Agreed, something like that sounds like it might have legs.


Shannon Deminick 09 Nov 2015, 13:51:23

PR for review is here: https://github.com/umbraco/Umbraco-CMS/pull/889

  • This adds an IAsyncRenderMvcController which can be used to implement a controller to have an async Index action without the hacks listed above
  • This means that the action lookup is still done in a simplistic way that uses MVC core to do the lookup
  • To test this you can create a document type called "Home" and create a controller that looks like:
     public class HomeController : Controller, IAsyncRenderMvcController
     {
         public async Task<ActionResult> Index(RenderModel model)
         {
             return await Task.FromResult((ActionResult)Content("hello async world"));
         }
     }
  • This action will execute for this controller with async functionality.


Kieron McIntyre 09 Nov 2015, 14:06:41

Great, that PR looks really clean. Thanks for taking the time to look at it.


Shannon Deminick 18 Nov 2015, 14:14:35

See notes here for review: http://issues.umbraco.org/issue/U4-7399#comment=67-24245 Its the same PR and testing notes for this issue and U4-7399


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions:

Due in version: 7.3.2

Sprint: Sprint 3

Story Points:

Cycle: