SmtpClient SendAsync error

Question

BACKGROUND

I have written a little console application that monitors a RabbitMQ queue for emails. Whenever an email is pushed onto the queue, my application would pick up that email, process it and send it.

CODE

Below is the code for my email service, it is what actually sends out the email.

public class MailService : IMailService
{
    private readonly SmtpClient _smtpClient = new SmtpClient();

    public void SendEmail(string toEmail, string subject, string body, bool isHtml)
    {
      var emailMessage = BuildEmailMessage(toEmail.Trim(), subject.Trim(), body, isHtml);
      _smtpClient.SendAsync(emailMessage, null);
    }

    #region Helpers
    private static MailMessage BuildEmailMessage(string toEmail, string subject, string body, bool isHtml)
    {
      const string fromEmailAddress = "james.brown@world.com";
      var emailMessage = new MailMessage(fromEmailAddress, toEmail, subject, body) { IsBodyHtml = isHtml };
      return emailMessage;
    }
    #endregion
}

PROBLEM

I had 2 emails on the RabbitMQ queue, and when I fired up my console application consumer, it threw the following exception after sending the first email (I received that 1st email in my inbox).

An asynchronous call is already in progress. It must be completed or canceled before you can call this method.

I did some digging around, and saw this thread which explains why I got this. Apparently using SendAsync() isn't the way to go because:

After calling SendAsync, you must wait for the e-mail transmission to complete before attempting to send another e-mail message using Send or SendAsync.

So what is the recommend way of going about this? I can create a new instance of the SmtpClient class for each email, but is that really a good idea?

Why are you using `SendAsync` in the first place? Just use `Send`. If you really need async, you should refactor your code and call `await SendMailAsync` which means you need to asyncify all the way up the call stack. — mason, Jul 13 '17 at 14:45
@mason you mean leave my code as it is, and just using `.Send(message)` or should I wrap it in a `using()` statement? — J86, Jul 13 '17 at 14:47
It would be better to wrap the `SmtpClient` in a using statement instead of keeping it as a field. But still, use `.Send(message)` instead of `SendAsync`, which is based on the old async patterns. — mason, Jul 13 '17 at 15:41
@mason that did it, please reply as an answer and I'll mark it as such. — J86, Jul 14 '17 at 09:29

mason · Accepted Answer · 2017-07-14T12:18:42.033

Send the email synchronously, there's no need to use the older async syntax of SendAsync. Also, ensure your SmtpClient only gets hit from one thread at a time by wrapping it in a using statement. There is a slight performance penalty, but you probably won't notice it unless you're sending a ton of emails. And if you are sending a ton, then overload your MailService.SendEmail method to accept an IEnumerable<EmailModel> and send them all at once using a single SmtpClient.

public void SendEmail(string toEmail, string subject, string body, bool isHtml)
{
    var emailMessage = BuildEmailMessage(toEmail.Trim(), subject.Trim(), body, isHtml);

    using(var client = new SmtpClient())
    {
        _smtpClient.Send(emailMessage);
    }      
}

//this would be the right way to do async
public async Task SendEmailAsync(string toEmail, string subject, string body, bool isHtml)
{
    var emailMessage = BuildEmailMessage(toEmail.Trim(), subject.Trim(), body, isHtml);

    using(var client = new SmtpClient())
    {
        _smtpClient.SendMailAsync(emailMessage);
    }      
}


//this would be the right way to do multiple emails
//you'd need to create an EmailModel class to contain all the details for each email (similar to MailMessage, but it would prevent your own code from taking a dependency on System.Net.Mail
public void SendEmail(IEnumerable<EmailModel> emailModels)
{

    var mailMessages = emailModels.Select(em =>  ConvertEmailModelToMailMessage(em));

    using(var client = new SmtpClient())
    {
        foreach(var mailMessage in mailMessages)
        {
            //you may want some error handling on the below line depending on whether you want all emails to attempt to send even if one encounters an error
            _smtpClient.Send(mailMessage);
        }
    }      
}

private MailMessage ConvertEmailModelToMailMessage(EmailModel emailModel)
{
    //do conversion here
}

SmtpClient SendAsync error

1 Answers1