19

I'm trying to set up email confirmation for an ASP.NET MVC5 website, based on the example AccountController from the VS2013 project template. I've implemented the IIdentityMessageService using SmtpClient, trying to keep it as simple as possible:

public class EmailService : IIdentityMessageService
{
    public async Task SendAsync(IdentityMessage message)
    {
        using(var client = new SmtpClient())
        {
            var mailMessage = new MailMessage("some.guy@company.com", message.Destination, message.Subject, message.Body);
            await client.SendMailAsync(mailMessage);
        }
    }
}

The controller code that is calling it is straight from the template (extracted into a separate action since I wanted to exclude other possible causes):

public async Task<ActionResult> TestAsyncEmail()
{
    Guid userId = User.Identity.GetUserId();
    
    string code = await UserManager.GenerateEmailConfirmationTokenAsync(userId);
    var callbackUrl = Url.Action("ConfirmEmail", "Account", new { userId = userId, code = code }, protocol: Request.Url.Scheme);
    await UserManager.SendEmailAsync(userId, "Confirm your account", "Please confirm your account by clicking <a href=\"" + callbackUrl + "\">here</a>");

    return View();
}

However I'm getting odd behavior when the mail fails to send, but only in one specific instance, when the host is somehow unreachable. Example config:

<system.net>
    <mailSettings>
        <smtp deliveryMethod="Network">
            <network host="unreachablehost" defaultCredentials="true" port="25" />
        </smtp>
    </mailSettings>
</system.net>

In that case, the request appears to deadlock, never returning anything to the client. If the mail fails to send for any other reason (e.g. host actively refuses connection) the exception is handled normally and I get a YSOD.

Looking at the Windows event logs, it seems that an InvalidOperationException is thrown around the same timeframe, with the message "An asynchronous module or handler completed while an asynchronous operation was still pending."; I get that same message in a YSOD if I try to catch the SmtpException in the controller and return a ViewResult in the catch block. So I figure the await-ed operation fails to complete in either case.

As far as I can tell, I am following all the async/await best practices as outlined in other posts on SO (e.g. HttpClient.GetAsync(...) never returns when using await/async), mainly "using async/await all the way up". I've also tried using ConfigureAwait(false), with no change. Since the code deadlocks only if a specific exception is thrown, I figure the general pattern is correct for most cases, but something is happening internally that makes it incorrect in that case; but since I'm pretty new to concurrent programming, I've a feeling I could be wrong.

Is there something I'm doing wrong ? I can always use a synchronous call (ie. SmtpClient.Send()) in the SendAsync method, but it feels like this should work as is.

David Klempfner
  • 8,700
  • 20
  • 73
  • 153
regexen
  • 193
  • 1
  • 7
  • 1
    Take a look at [Stephen Cleary's answer on catching an exception on a void method(`SendMailAsync`)](http://stackoverflow.com/a/7350534/209259). Async Void Methods are problem childs sometimes. – Erik Philips Feb 04 '15 at 23:19
  • 1
    @ErikPhilips - I don't see any `async void` methods in the sample (either implemented or called) - did you mean some particular line? – Alexei Levenkov Feb 04 '15 at 23:42
  • 1
    As workaround you may try to manually resolve the host and fail earlier... Also look at [the source](http://referencesource.microsoft.com/#System/net/System/Net/mail/SmtpClient.cs,b9a40a3be18a4d58) for insights - hopefully it would be helpful... – Alexei Levenkov Feb 04 '15 at 23:46
  • @regexen: Do you have [`httpRuntime.targetFramework` set to `4.5` in your `web.config`](http://blogs.msdn.com/b/webdev/archive/2012/11/19/all-about-httpruntime-targetframework.aspx)? – Stephen Cleary Feb 05 '15 at 01:51
  • @StephenCleary: Yup. – regexen Feb 05 '15 at 14:12
  • 2
    I remember a related question with a workaround... there it is: [Sending async mail from SignalR hub](http://stackoverflow.com/q/24377081) – noseratio Feb 06 '15 at 00:46
  • @Noseratio: Thanks for the suggestion. I tried that solution, and now the exception seems to be swallowed by the `IgnoreSynchronizationContext`, and the controller method returns as if nothing went wrong. Since I can't handle faults in that case, it doesn't fix it for me. – regexen Feb 06 '15 at 15:29
  • 1
    @regexen, try my `WithNoContext` from [here](http://stackoverflow.com/a/24386344/1768303), see if it makes any difference. – noseratio Feb 08 '15 at 11:04
  • @Noseratio: Seems to work the same way; the exception is not caught outside the helper method. – regexen Feb 10 '15 at 16:16
  • Thanks a lot Noseratio and Stephen!, fixed the same issue for me...Now I have to learn more about EAP – Practical Programmer Jun 17 '17 at 21:54

1 Answers1

15

Try this implementation, just use client.SendMailExAsync instead of client.SendMailAsync. Let us know if it makes any difference:

public static class SendMailEx
{
    public static Task SendMailExAsync(
        this System.Net.Mail.SmtpClient @this,
        System.Net.Mail.MailMessage message,
        CancellationToken token = default(CancellationToken))
    {
        // use Task.Run to negate SynchronizationContext
        return Task.Run(() => SendMailExImplAsync(@this, message, token));
    }

    private static async Task SendMailExImplAsync(
        System.Net.Mail.SmtpClient client, 
        System.Net.Mail.MailMessage message, 
        CancellationToken token)
    {
        token.ThrowIfCancellationRequested();

        var tcs = new TaskCompletionSource<bool>();
        System.Net.Mail.SendCompletedEventHandler handler = null;
        Action unsubscribe = () => client.SendCompleted -= handler;

        handler = async (s, e) =>
        {
            unsubscribe();

            // a hack to complete the handler asynchronously
            await Task.Yield(); 

            if (e.UserState != tcs)
                tcs.TrySetException(new InvalidOperationException("Unexpected UserState"));
            else if (e.Cancelled)
                tcs.TrySetCanceled();
            else if (e.Error != null)
                tcs.TrySetException(e.Error);
            else
                tcs.TrySetResult(true);
        };

        client.SendCompleted += handler;
        try
        {
            client.SendAsync(message, tcs);
            using (token.Register(() => client.SendAsyncCancel(), useSynchronizationContext: false))
            {
                await tcs.Task;
            }
        }
        finally
        {
            unsubscribe();
        }
    }
}
noseratio
  • 59,932
  • 34
  • 208
  • 486
  • 3
    That one works; the exception is caught as one would normally expect, bubbles up the call stack and I get a YSOD. Seems like a whole lot of code to do something that seemed so simple (!), but I can see how it can get complicated fast. Anyway marking as accepted since it does solve it. Thanks for all your help! – regexen Feb 13 '15 at 23:36
  • Glad it works; it's a typical `Task`-based wrapper over [EAP pattern](https://msdn.microsoft.com/en-us/library/ms228969%28v=vs.110%29.aspx). I wonder if it still would work *without* `await Task.Yield()`, maybe you could give it a try. – noseratio Feb 13 '15 at 23:52
  • 1
    Seems to work without the `Task.Yield`, according to a quick test. – regexen Feb 20 '15 at 14:40
  • @regexen, in which case, the `handler` doesn't need to be async, apparently. – noseratio Feb 20 '15 at 14:44
  • So is there a bug in SmtpClient that this code works around? – pettys Mar 20 '16 at 05:06
  • @pettys, there might a bug but I couldn't spot it from the first glance. – noseratio Mar 20 '16 at 05:35
  • Why don't you use `Task.Run(async () => await SendMailExImplAsync(@this, message, token));`? (By your current implementation you're still 'wasting' a thread while doing the IO operation, The way I suggested you're freeing up the thread until IO operation is completed) – BornToCode Jan 03 '21 at 21:01
  • 1
    *By your current implementation you're still 'wasting' a thread while doing the IO operation...* @BornToCode I'm not. `Task.Run(() => FuncAsync())` is basically doing the same as `Task.Run(async () => await FuncAsync())` but without added async state machine micro-overhead. In both cases, the same [`Task.Run` override](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.run?view=net-5.0#System_Threading_Tasks_Task_Run_System_Func_System_Threading_Tasks_Task__) is used. – noseratio Jan 03 '21 at 22:02
  • But without await the thread is held and not released to the thread pool even while you're waiting for the IO operation of sending the email. – BornToCode Jan 04 '21 at 10:34
  • 1
    @BornToCode, no `Task.Run` won't block anything for functions that return a `Task` like `SendMailExImplAsync` (unless such function has a blocking wait inside, which mine doesn't). If you like to learn more, have a look at [`Task.Run` implementation](https://source.dot.net/#System.Private.CoreLib/Task.cs,c3e0ffe470f08b7b). – noseratio Jan 04 '21 at 10:58
  • This appears to work for the same issue I was experiencing, however it seems to only time-out and return after a full 5 minutes - even though the TimeOut on the SmtpClient is set to 60000. Is there any way to reduce the time-out? – Gary Oct 28 '21 at 14:01
  • 1
    @Gary, create a [`CancellationTokenSource` with timeout](https://learn.microsoft.com/en-us/dotnet/api/system.threading.cancellationtokensource.-ctor?view=net-5.0#System_Threading_CancellationTokenSource__ctor_System_Int32_) you want and pass its token to `SendMailExImplAsync` – noseratio Oct 28 '21 at 19:49
  • 1
    @noseratio. Perfect, this seems to work nicely. – Gary Oct 29 '21 at 06:59