7

I'm having trouble getting my head round an issue I'm having with async / await.

in a nutshell, I have a controller, decorated with an attribute. This attribute gets the specified content from an i/o intensive process (filesystem / db / api etc...)

It then sets the returned content as a Dictionary<string, string> on the ViewBag

Then, in a view, I can do something like this, to retrieve the content:

@(ViewBag.SystemContent["Common/Footer"])

The issue I'm having, is the first time it runs, the content hasn't returned, and the call to retrieve the value by string index fails, as it doesn't exist.
Hit F5, and it's fine.

Controller action is pretty simple:

[ProvideContent("Common/Footer")]
public class ContactDetailsController : Controller
{
    public async Task<ActionResult> Index()
    {
        //omitted for brevity - awaits some other async methods
        return View();
    }
}

The attribute

public override async void OnActionExecuting(ActionExecutingContext filterContext)
{
    if (filterContext.Result is ViewResult)
    {
        var localPath = filterContext.RouteData.Values["controller"] + "/" + filterContext.RouteData.Values["action"];

        if (!_useControllerActionAsPath)
            localPath = _path;

        var viewResult = filterContext.Result as ViewResult;

        //this seems to go off and come back AFTER the view requests it from the dictionary
        var content = await _contentManager.GetContent(localPath);

        if (viewResult.ViewBag.SystemContent == null)
            viewResult.ViewBag.SystemContent = new Dictionary<string, MvcHtmlString>();

        viewResult.ViewBag.SystemContent[localPath] = new DisplayContentItem(content, localPath);
    }

EDIT

Changing the following line in my Attribute:

var content = await _contentManager.GetContent(localPath);

To

var content = Task.Factory.StartNew(() =>
            manager.GetContent(localPath).Result, TaskCreationOptions.LongRunning).Result;

solves the problem, but I feel it goes against everything I've read on Stephen Clearys blog...

Alex
  • 37,502
  • 51
  • 204
  • 332
  • i'm no async expert, but you're defining content and using it right away, can't you just change it to a synchronous call? – DLeh Apr 08 '15 at 15:47
  • with content, I'm *awaiting* _contentmanager.GetContent - which is asynchronous..... even if i block with a .Result on the end, it's the same end result – Alex Apr 08 '15 at 15:50
  • I suspect the problem is that you're overriding a synchronous method with an async one. If you need it to run to completion before the action executes, why have you made it async? – Lee Apr 08 '15 at 15:53
  • why have I made which async? The overridden method? – Alex Apr 08 '15 at 15:53
  • Yes, `Controller.OnActionExecuting` looks like a synchronous method. If you make yours async then it could still be running when the `Index` action completes. – Lee Apr 08 '15 at 15:55
  • i made it async to await the call to _contentManager.GetContent(localPath) – Alex Apr 08 '15 at 16:01

1 Answers1

9

I'm not 100% familiar with the ASP.Net MVC stack and how all this works, but I'm going to take a stab at it.

The OnActionExecuting() documentation says:

Called before the action method is invoked.

Since you overrode a previously synchronous method and made it asynchronous, it's expecting that code to be complete, and ready for the next execution step.

Theoretical execution path:

public void ExecuteAction()
{
 OnActionExecuting();

 OnActionExecution();

 OnActionExecuted();
}

Since you overrode the OnActionExecuting() method, the execution stack is basically still the same, but the next code to be executed (ExecuteAction() and OnActionExecuted() and whatever called ExecuteAction()) were expecting an synchronous calls to be made, so to their knowledge everything is fine and ready to keep on running.

Basically, this boils down to OnActionExecuting() isn't an asynchronous method, and nothing is expecting it to be. (It's not asynchronous in MVC 6 either.)

Something, that's being called synchronously after OnActionExecuting() and it's sequential calls, is referencing viewResult.ViewBag.SystemContent and thus it's not getting the value you're wanting. As you said yourself in the title,

Awaited Async method not returning before result is required.

The 'catch' with using Tasks is that you can't guarantee when the task will complete, but you are guaranteed that it will complete.

Potential solutions:

  • Move the GetContent() call out of that event.
  • Store off the Task that's created for GetContent(), find the next place that your viewResult.ViewBag.SystemContent is used and check for the task completion or wait on the completion.
  • Add a timeout interval to the GetContent() method. (Tons of different ways to do this. MSDN Docs for Task class This won't fix your problem.

Edit: Code sample for storing Task in controller

[ProvideContent("Common/Footer")]
public class ContactDetailsController : Controller
{

/*
 * BEGINNING OF REQUIRED CODE BLOCK
 */
    private Task<string> _getContentForLocalPathTask; 
    private string _localPath;

/*
 * END OF REQUIRED CODE BLOCK
 */
    public async Task<ActionResult> Index()
    {
        //omitted for brevity - awaits some other async methods
/*
 * BEGINNING OF REQUIRED CODE BLOCK
 */
        string content = await _getContentForLocalPath;

        viewResult.ViewBag.SystemContent[_localPath] = new DisplayContentItem(content, _localPath);            
/*
 * END OF REQUIRED CODE BLOCK
 */

         return View();
    }

public override async void OnActionExecuting(ActionExecutingContext filterContext)
{
    if (filterContext.Result is ViewResult)
    {
        var localPath = filterContext.RouteData.Values["controller"] +
                         "/" + filterContext.RouteData.Values["action"];

        if (!_useControllerActionAsPath)
            localPath = _path;

        var viewResult = filterContext.Result as ViewResult;

/*
 * BEGINNING OF REQUIRED CODE BLOCK
 */
       _localPath = localPath;

       _getContentForLocalPathTask = _contentManager.GetContent(localPath);
/*
 * END OF REQUIRED CODE BLOCK
 */

        if (viewResult.ViewBag.SystemContent == null)
            viewResult.ViewBag.SystemContent = new Dictionary<string, MvcHtmlString>();

    }
}
Cameron
  • 2,574
  • 22
  • 37
  • 3
    +1. The first rule of `async` is to avoid `async void`. In this case, `OnActionExecuting` returns to ASP.NET when it hits its `await`, causing the request pipeline to continue. I'd recommend storing the asynchronous operation in a `Task` (i.e., in your controller type) and `await`ing that task within your controller action. – Stephen Cleary Apr 08 '15 at 17:44
  • @StephenCleary - not sure I follow re: storing the Task in the controller action? My ProvideContent attribute is currently on the Controller itself, not the ActionMethod - I'm open to suggestions of binning this idea though, but it seems like a logical approach? – Alex Apr 08 '15 at 18:51
  • 1
    @Alex I have updated my answer with what Stephen was referring to. – Cameron Apr 08 '15 at 19:08
  • @Cameron thanks, but this doesn't make sense - OnActionExecuting is part of the attribute, yet you have it within the Controller body? i may have to look into making the content retrieval sync, as i've inherited this, and the above change is a pretty big one (editing every controller this is used) – Alex Apr 09 '15 at 08:00
  • Just read @StephenCleary answer here - http://stackoverflow.com/questions/12482338/async-action-filter-in-mvc-4 - so i guess in my case blocking the async call would be the right thing to do in this instance? – Alex Apr 09 '15 at 08:33
  • See my edit - this seems to fix it, but as i say, feels wrong – Alex Apr 09 '15 at 10:10
  • @Alex: OK, I think I see the problem: you're talking about `ActionFilterAttribute.OnActionExecuting` and we thought you were talking about `Controller.OnActionExecuting`. I'd still recommend setting a property on the `filterContext.Controller` instead of blocking. If a public property feels evil you can hide it with an explicitly implemented interface. – Stephen Cleary Apr 09 '15 at 12:33
  • @Alex Sorry, I didn't get notification that you had commented more. You could simplify that line of code down to `var task = manager.GetContent(localPath).Wait();` and `var content = task.Result;` to be cleaner. – Cameron Apr 09 '15 at 12:37
  • @Alex: Like [this gist](https://gist.github.com/StephenCleary/7deeebf37c91fcf06a30). And yes, it's awkward because MVC doesn't support asynchronous filters. This has already been fixed in ASP.NET vNext. – Stephen Cleary Apr 09 '15 at 12:42