Is this REALLY async email AND best practice?

Question

I am trying to learn and master ASYNC and AWAIT but it is not clear if what I do is the right thing at then end.. I mean the code works and I took two browsers/instances of the same form and click, well, almost at the same time and everything went OK. I received 2 emails but is it the right way to do it? and does it really gives a plus value to do it async instead of sync...

This is my code:

    [HttpPost]
    [AllowAnonymous]
    [ValidateAntiForgeryToken]
    public async Task<ActionResult> Register(RegisterViewModel model)
    {
       bla bla do some sync simple stuffs etc...
       ...
       await Task.Run(() => Utility.MailUtility.SendEmail(user.Email, user.To, msg, subj));
       return RedirectToAction("Index", "Home");
    }

and then the mailing code looks like this, as per a sample code taken from stackoverflow:

    public static async Task SendEmail(params etc...) 
    {
        await SendMailForTemplateID(params etc...);
    }

    private static Task SendMailForTemplateID(params etc...)
    {


            var task = Task.Factory.StartNew(() =>
            {
                MailMessage message = new MailMessage();
                MailMessage mail = new MailMessage();
                //SmtpClient SmtpServer = new SmtpClient(ConfigurationManager.AppSettings["SMTPHost"]);
                SmtpClient SmtpServer = new SmtpClient("localhost"); // for test purpose only
                mail.From = new MailAddress(from);
                mail.To.Add(to);
                mail.Subject = GetSubjectForTemplateId(templateID, extraMsg, dataInfo, fromName, toName, isFr);
                mail.Body = GetBodyForTemplateId(templateID, extraMsg, dataInfo, fromName, toName, isFr);
                mail.IsBodyHtml = false; 
                SmtpServer.Port = 81; // for test purpose only
                SmtpServer.Credentials = new System.Net.NetworkCredential
                    (ConfigurationManager.AppSettings["CredentialUser"],
                    ConfigurationManager.AppSettings["CredentialPassword"]);

                bool result;
                bool.TryParse(ConfigurationManager.AppSettings["EnableSSL"], out result);
                SmtpServer.EnableSsl = result;

                SmtpServer.Send(mail);

                //* make sure we leave nothing in memory
                message.Dispose();
                mail.Dispose();
                SmtpServer.Dispose();


            });

            return task;

       }

---

I am sorry guys but I don't know how to put line return (enter) between my replies. Is It possible? nway, regarding ASYNC-EMAIL best practice.

My new version of code looks like this, it works more simple coding but is it right? I mean no new threads, does it give a plus value VS sync sendmail methods etc...

I call it in a MVC action. The code:

     do stuff...
     await Utility.MailUtility.SendMailForTemplateID(params);
     do stuff...

   public static async Task SendMailForTemplateID(params...)
    {
        try
        {
            email code etc...

            await smtpClient.SendMailAsync(mail);

            mail.Dispose();
            smtpClient.Dispose();


        }
        catch (Exception)
        {
            // Log ex
        }
    }

Answer 1

No, wrapping async calls into Task.Run is wrong way of writing async/await code.

One particular problem with your current code is Task.Run and Task.Factory.StartNew start new threads to execute operation - so code starts 2 extra threads to execute operation.

Correct way - remove all Task.Run/Task.Factory.StartNew calls and await asynchronous calls all the way down. In your case SmtpClient.SendMailAsync should be used at lowest level instead of SmtpServer.Send(mail);.

In more generic case when there is no Task-based API but there is some alternative asynchronous APIs (like event-based SmtpClient.SendAsync ) you may need to provide wrapping helper methods to transform such calls to Task-based async methods. Some approaches shown in Convert Event based pattern to async CTP pattern

---

Approximate code:

public async Task<ActionResult> Register(RegisterViewModel model)
{
   ...
   await Utility.MailUtility.SendEmailAsync(user.Email, user.To, msg, subj));
   return RedirectToAction("Index", "Home");
}

// No need to add `async` here as there is no `await` inside,
// possibly can be removed altogether.
public static Task SendEmailAsync(params etc...) 
{
    return SendMailForTemplateID(params etc...);
}

private async static Task SendMailForTemplateID(params etc...)
{
     // not Task.Run / Task.Factory.StartNew

     using (MailMessage message = ... )
     using (SmtpClient smtp = ....)
     {
            .... // setup message/smtp

            await smtp.SendMailAsync(mail); // Task-based Send
     }
     // no need to return anything for `Task` (essentially `void`) method 
   }