MVC App - Is this the correct approach for using Async and Await?

Question

I have an MVC controller method that does a number of things, but the last thing it does is this:

    public void PerformCheckout(int salesAddressId)
    {
        try
        {
            ...
            ...
            // We just made a sale. Send emails to all market owners.
            SendSalesEmailsAsync(master);
        }
        catch (System.Exception exception)
        {
            // Log the error
        }

And then SendSalesEmailesAsynch() looks like this:

    private async Task SendSalesEmailsAsync(SalesMaster salesMaster)
    {
        ...
        ...
        // Send an email to the owner of each marker
        foreach(string market in markets)
            await SendSalesEmailAsync(salesMaster, salesDetailList, market).ConfigureAwait(false);
    }

SendSalesEmailAsynch() looks like this:

    // Send an email to a market owner
    private async Task SendSalesEmailAsync(SalesMaster salesMaster, List<SalesDetail> salesDetail, string marketName)
    {
        ...
        ...
        Email email = new Email(new List<string> { sellerEmailAddress }, emailSubject, emailHtmlBody);
        await email.SendAsync().ConfigureAwait(false);

And finally, the method that actually sends the email:

    public async Task SendAsync()
    {
        // Create a network credentials object
        var credentials = new NetworkCredential(azureUserName, azurePassword);

        // Create an Web transport for sending the email
        var transportWeb = new Web(credentials);

        // Send the email. We don't care about the current context so let's run this in a thread pool context.
        // For more information: http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html
        await transportWeb.DeliverAsync(this._email).ConfigureAwait(false);
    }

I am pretty new with Async and Await. Does all this look proper? Also, I'm not entirely sure that the catch block in the controller action method will get called if an error occurs while sending an email.

Is this question similar enough to help: http://stackoverflow.com/questions/27798039/is-this-the-correct-usage-of-async-await-in-mvc-with-service-repository-layer — neontapir, May 11 '15 at 22:18
@neontapir - Maybe but I'm not sure. I don't really know enough about async / await to know how much of that applies. I suspect there are one or more experts here that can quickly look at my situation and tell me for sure. — Randy Minder, May 11 '15 at 22:20
I can look at it, and tell you that there are some issues here :P Explaining it in a full fledged answer with citations is where the quickly part becomes more complex. — Travis J, May 11 '15 at 22:21
If you want to have more open discussion on this topic, please join the [c# chat room](http://chat.stackoverflow.com/rooms/7/c) — Travis J, May 11 '15 at 22:35
This question belongs on the CodeReview site. http://codereview.stackexchange.com/ — Johnathon Sullinger, May 11 '15 at 22:53
@JohnathonSullinger This code isn't complete, there are plenty of `...` sections in the code, this question would not be well received on codereview. — Ethan Bierlein, May 11 '15 at 22:55
@EthanBierlein That doesn't mean that the question needed to be posted in its current form to codereview. A small number of changes is all that would be needed in order to pose this question to that site and get it reviewed. — Johnathon Sullinger, May 11 '15 at 22:58
@JohnathonSullinger Then perhaps point the user to the [help center](http://codereview.stackexchange.com/help) for the site you're recommending them to. — nhgrif, May 11 '15 at 22:59
@EthanBierlein - Thanks but I'm not asking for a thorough code review. I was just asking if my implementaton of async / await is flawed. — Randy Minder, May 11 '15 at 23:00
@RandyMinder Yep, I know, I was just making sure that `@JohnathonSullinger knew about what was on and off-topic for codereview. — Ethan Bierlein, May 11 '15 at 23:01

score 1 · Accepted Answer · answered May 11 '15 at 22:24

1

As you suspected, the catch block won't get any exceptions since you didn't synchronize with current context. In order to do that you have to mark your action method as async and use await for calling SendSalesEmailsAsync. So the action method will look like this:

public async Task PerformCheckout(int salesAddressId)
{
    try
    {
        ...
        ...
        // We just made a sale. Send emails to all market owners.
        await SendSalesEmailsAsync(master);
    }
    catch (System.Exception exception)
    {
        // Log the error
    }

answered May 11 '15 at 22:24

Arin Ghazarian

5,105
3
23
21

Thank you! Other than that, everything else looks appropriate? – Randy Minder May 11 '15 at 22:26
Yes @RandyMinder every other thing looks perfectly fine, you've used async all the way down and also used `ConfigureAwait` to avoid deadlock for non await calls. – Arin Ghazarian May 11 '15 at 22:29

score 1 · Answer 2 · answered May 11 '15 at 22:35

This is not the typical approach used for async await. I would not feel comfortable saying "incorrect" because design goals are so implementation dependent.

There are several issues that show up in the code shown. Having a method which takes an async action and returns void is probably a bad idea for a few reasons.

Controllers shouldn't really be exposing functionality that doesn't include returning information. If it is an action to perform, it should be called as some sort of service or business logic. Furthermore, since the method is public and part of the controller anyone can invoke it from the url. As it is sending a group of emails, this may be undesirable.

Async/await should be used to free up processing power. For the most part, it should only be used when either the processing time or waiting time exceeds 40 milliseconds. As emails are fire and forget, this may not be the case in your situation in which case the entire method could be converted back to synchronous.

If it is taking longer than 40ms, then async is going to be okay to use. However, in the shown code the first call is not properly used. When an async call is made without await it is basically "fire and forget". Especially when there is no return value, the return is never noticed. As a result, if an exception is thrown it is also forgotten, and the catch block will never executed.

Using await causes a new task continuation to be created. The rest of the method is essentially forked while the call takes place. Once the called execution has finished, then the rest of the method which was forked executes. Without await, the rest of the method simply executes, and the execution of the side call probably finishes far later.

All in all, this design should probably be refactored to be called from a more controlled environment, and possibly even from a synchronous method.

I disagree with several things you state and I also wonder why you even mentioned them since they are not germane to the question being asked. Having a controller method return void is perfectly acceptable. And, it's also false that anyone can call the method and send out emails. SInce you're not seeing the entirety of the method, you cannot see this would be impossible. But it's also not relevent to the question. — Randy Minder, May 11 '15 at 23:06
@RandyMinder - You asked about an approach. Async bubbles out, so the approach used in your controller design is relevant. Having a controller method return void is bad practice. A public method in a controller is publicly accessible. If you have an authorize attribute, then it is still accessible to users who are logged in. Basically, any time this action is caused by a user, they would also be able to invoke the method by using the url. That you have extra business logic not shown in your question but still present just shows the level of coupling which would need to be refactored here. — Travis J, May 11 '15 at 23:37

MVC App - Is this the correct approach for using Async and Await?

2 Answers2