Why do we periodically get "An asynchronous call is already in progress" when calling SmtpClient.Send?

Question

We have some (synchronous) email code, which creates a class that creates an SmtpClient, and then sends an email. The SmtpClient is not reused; however we get the following exception every now and then:

System.Web.HttpUnhandledException (0x80004005): Exception of type 'System.Web.HttpUnhandledException' was thrown. ---> System.InvalidOperationException: An asynchronous call is already in progress. It must be completed or canceled before you can call this method.
   at System.Net.Mail.SmtpClient.Send(MailMessage message)
   at EmailSender.SendMail(MailAddress fromMailAddress, string to, String subject, String body, Boolean highPriority) in ...\EmailSender.cs:line 143

Code looks like this:

// ...
var emailSender = new EmailSender();
emailSender.SendMail(toEmail, subject, body, true);
// emailSender not used past this point
// ...

public class EmailSender : IEmailSender
{
    private readonly SmtpClient smtp;

    public EmailSender()
    {
        smtp = new SmtpClient();
    }

    public void SendMail(MailAddress fromMailAddress, string to, string subject, string body, bool highPriority)
    {
        if (fromMailAddress == null)
            throw new Exception();
        if (to == null)
            throw new ArgumentException("No valid recipients were supplied.", "to");

        // Mail initialization
        var mailMsg = new MailMessage
        {
            From = fromMailAddress,
            Subject = subject,
            Body = body,
            IsBodyHtml = true,
            Priority = (highPriority) ? MailPriority.High : MailPriority.Normal
        };

        mailMsg.To.Add(to);


        smtp.Send(mailMsg);
    }
}

score 5 · Answer 1 · answered May 08 '13 at 10:27

5

You need to dispose of the SmtpClient using Dispose, using or by implementing the disposable pattern for your class EmailSender (which more appropriate here because you are tying the lifetime of the SmtpClient to the lifetime of the EmailSender in the constructor.)

That might solve this exception.

answered May 08 '13 at 10:27

Emond

50,210
11
84
115

I agree we should Dispose, but could it really cause this problem? – Danny Tuppeny May 08 '13 at 12:27
It might. The "Every now and then" from your question might be related to timeouts, cleaning up, ... Add the correct resource management and see if the exception still shows up. It is hard to pinpoint the problem if you can't reproduce it. – Emond May 08 '13 at 12:40

jgauffin · Answer 2 · 2013-05-08T10:38:31.660

My guess is that the SmtpClient was not designed to send multiple messages concurrently.

I would change the class like this instead:

public class EmailSender : IEmailSender
{
    Queue<MailMessage> _messages = new Queue<MailMessage>();
    SmtpClient _client = new SmtpClient();

    public EmailSender()
    {
    }

    public void SendMail(MailAddress fromMailAddress, string to, string subject, string body, bool highPriority)
    {
        if (fromMailAddress == null)
            throw new ArgumentNullException("fromMailAddress");
        if (to == null)
            throw new ArgumentException("No valid recipients were supplied.", "to");

        // Mail initialization
        var mailMsg = new MailMessage
        {
            From = fromMailAddress,
            Subject = subject,
            Body = body,
            IsBodyHtml = true,
            Priority = (highPriority) ? MailPriority.High : MailPriority.Normal
        };

        mailMsg.To.Add(to);

        lock (_messages)
        {
            _messages.Enqueue(mailMsg);
            if (_messages.Count == 1)
            {
                ThreadPool.QueueUserWorkItem(SendEmailInternal);
            }
        }
    }

    protected virtual void SendEmailInternal(object state)
    {
        while (true)
        {
            MailMessage msg;
            lock (_messages)
            {
                if (_messages.Count == 0)
                    return;
                msg = _messages.Dequeue();
            }

            _client.Send(msg)
        }
    }
}

As there are really no reason to create the client in the constructor.

I also changed so that the class throws ArgumentNullException and not Exception if fromMailAddress is null. An empty Exception doesn't say much..

Update

The code do now use a thread pool thread for the sending (and reusing the smtpclient).

Or rather than creating a new SmtpClient instance, the OP could just lock that object until the message is sent. — MeTitus, May 08 '13 at 10:29
Yes there is a good reason to create the client in the constructo: when you are sending multiple emails to the same server connections will be pooled. See the remarks section on http://msdn.microsoft.com/en-us/library/system.net.mail.smtpclient.dispose.aspx — Emond, May 08 '13 at 10:31
I don't understand why we need locks. We are not sharing instances of either class. The EmailSender class is created; it's SendEmail method is called, and then it is finished with. Nothing is static. — Danny Tuppeny, May 08 '13 at 12:26
You didn't say antyhing about not sharing, because that's what it seems like on the error message — jgauffin, May 08 '13 at 18:19
My question says "The SmtpClient is not reused" and in the code sample has a comment "// emailSender not used past this point" :( — Danny Tuppeny, May 09 '13 at 17:24

Why do we periodically get "An asynchronous call is already in progress" when calling SmtpClient.Send?

2 Answers2