8

I have a windows forms application in which I send an email using SmtpClient. Other async operations in the application use async/await, and I'd ideally like to be consistent in that when sending the mail.

I display a modal dialog with a cancel button when sending the mail, and combining SendMailAsync with form.ShowDialog is where things get tricky because awaiting the send would block, and so would ShowDialog. My current approach is as below, but it seems messy, is there a better approach to this?

private async Task SendTestEmail()
{
  // Prepare message, client, and form with cancel button
  using (Message message = ...)
  {
     SmtpClient client = ...
     CancelSendForm form = ...

     // Have the form button cancel async sends and
     // the client completion close the form
     form.CancelBtn.Click += (s, a) =>
     {
        client.SendAsyncCancel();
     };
     client.SendCompleted += (o, e) =>
     {
       form.Close();
     };

     // Try to send the mail
     try
     {
        Task task = client.SendMailAsync(message);
        form.ShowDialog();
        await task; // Probably redundant

        MessageBox.Show("Test mail sent", "Success");
     }
     catch (Exception ex)
     {
        string text = string.Format(
             "Error sending test mail:\n{0}",
             ex.Message);
        MessageBox.Show(text, "Error");
     }
  }   
FlintZA
  • 872
  • 1
  • 11
  • 22
  • The seems like a really neat solution. You are correct that the `await` is not required but it is nice and readable. – Gusdor Apr 13 '15 at 12:07
  • Thanks, I tried the solution you added and then removed. But form.Show(this) (where this is the main form) no longer treats the main form as parent so I lose the behavior from form.StartPosition = FormStartPosition.CenterScreen that I didn't include above. Attempting to then set the parent to the current form throws an exception about form being top level, and form.TopLevel is read only (rabbit hole! :)) – FlintZA Apr 13 '15 at 12:14
  • I removed the answer because i was incorrect :) – Gusdor Apr 13 '15 at 12:22
  • You are using form.ShowDialog() when you don't really need a modal window i.e. you dont use the DialogResult. Why not just show the form, prohibit closing it and use it to display your error text (if applicable) and cancel button? – Simon Apr 13 '15 at 13:28
  • 1
    Slightly off topic `await task;` is not redundant, that is how you make sure any exceptions thrown by `SendMailAsync` will get caught by your try/catch block. – Scott Chamberlain Apr 13 '15 at 13:35
  • What do you do if the user hits Cancel? `SendMailAsync` doesn't accept cancellation token. Are you using `SendAsyncCancel` (which is intended for `SendAsync`)? – noseratio Apr 13 '15 at 13:36
  • 1
    @Noseratio The MSDN is just poorly documented, `SendMailAsync` [is just a TaskCompletionSource wrapper](http://referencesource.microsoft.com/System/R/b9a40a3be18a4d58.html) around the EAP `SendAsync` method. Calling `SendAsyncCancel` [will cancel](http://referencesource.microsoft.com/System/R/d0ba4bd6ac9dcff9.html) `SendMailAsync` – Scott Chamberlain Apr 13 '15 at 13:42
  • Thanks @ScottChamberlain I wasn't aware that await was what would be needed for the exceptions, good to know. I am using SendAsyncCancel, you can see in the listing that it's being called from a button click handler on the form. – FlintZA Apr 13 '15 at 13:51
  • @Simon the form is already borderless (so can't be closed), it's only job is to indicate to the user what is happening, and allow them to cancel sending if it is taking too long. It is displayed as a dialog because I don't want the user able to interact with the parent form in any way. – FlintZA Apr 13 '15 at 13:54
  • @Noseratio I'm aware that SendMailAsync doesn't handle a cancellationtoken. I've actually added an extension that fixes that because in the service itself (this form is just to allow configuration of settings for a windows service) I need "proper" async support: https://gist.github.com/mattbenic/400e3c039ab8ea3e33aa – FlintZA Apr 13 '15 at 14:02

2 Answers2

10

I would consider handling the Form.Shown event and sending the email from there. Since it'll fire asynchronously, you don't need to worry about "working around" ShowDialog's blocking nature, and you have a slightly cleaner way to synchronize closing the form and showing the success or failure message.

form.Shown += async (s, a) =>
{
    try
    {
        await client.SendMailAsync(message);
        form.Close();
        MessageBox.Show("Test mail sent", "Success");
    }
    catch(Exception ex)
    {
        form.Close();
        string text = string.Format(
            "Error sending test mail:\n{0}",
            ex.Message);
        MessageBox.Show(text, "Error");
    }
};

form.ShowDialog();
Todd Menier
  • 37,557
  • 17
  • 150
  • 173
  • 1
    This doesn't really do much to add to readability. What it _does_ do is fix the race condition bug that was in the original code, in which if the operation completed before the dialog was even shown (an admittedly small risk, but one not logically precluded by the implementation), the `Close()` method would be called at the wrong time (i.e. before the dialog was shown). `try`/`catch` handling can be moved to the `Shown` event handler to capture exceptions thrown by `await`. – Peter Duniho Apr 13 '15 at 15:54
  • Agree with all points. Updated my code - try/catch appears to deal with email sending failures and definitely belongs in the event handler. – Todd Menier Apr 13 '15 at 16:10
  • Thanks, I think you're right this is a better approach to the specific problem in the original listing. I also like that it handles the minor chance of the failed call issue as @Peter Duniho mentioned. After your edits I also think it's as least as readable as the original if not more so. – FlintZA Apr 14 '15 at 06:15
1

One questionable thing about your existing SendTestEmail implementation is that it's in fact synchronous, despite it returns a Task. So, it only returns when the task has already completed, because ShowDialog is synchronous (naturally, because the dialog is modal).

This can be somewhat misleading. For example, the following code wouldn't work the expected way:

var sw = new Stopwatch();
sw.Start();
var task = SendTestEmail();
while (!task.IsCompleted)
{
    await WhenAny(Task.Delay(500), task);
    StatusBar.Text = "Lapse, ms: " + sw.ElapsedMilliseconds;
}
await task;

It can be easily addressed with Task.Yield, which would allow to continue asynchronously on the new (nested) modal dialog message loop:

public static class FormExt
{
    public static async Task<DialogResult> ShowDialogAsync(
        Form @this, CancellationToken token = default(CancellationToken))
    {
        await Task.Yield();
        using (token.Register(() => @this.Close(), useSynchronizationContext: true))
        {
            return @this.ShowDialog();
        }
    }
}

Then you could do something like this (untested):

private async Task SendTestEmail(CancellationToken token)
{
    // Prepare message, client, and form with cancel button
    using (Message message = ...)
    {
        SmtpClient client = ...
        CancelSendForm form = ...

        // Try to send the mail
        var ctsDialog = CancellationTokenSource.CreateLinkedTokenSource(token);
        var ctsSend = CancellationTokenSource.CreateLinkedTokenSource(token);
        var dialogTask = form.ShowDialogAsync(ctsDialog.Token);
        var emailTask = client.SendMailExAsync(message, ctsSend.Token);
        var whichTask = await Task.WhenAny(emailTask, dialogTask);
        if (whichTask == emailTask)
        {
            ctsDialog.Cancel();
        }
        else
        {
            ctsSend.Cancel();
        }
        await Task.WhenAll(emailTask, dialogTask);
    }   
}

public static class SmtpClientEx
{
    public static async Task SendMailExAsync(
        SmtpClient @this, MailMessage message, 
        CancellationToken token = default(CancellationToken))
    {
        using (token.Register(() => 
            @this.SendAsyncCancel(), useSynchronizationContext: false))
        {
            await @this.SendMailAsync(message);
        }
    }
}
noseratio
  • 59,932
  • 34
  • 208
  • 486
  • @ScottChamberlain, thanks for pointing out `SendAsyncCancel` is still usable with `SendMailAsync`, albeit undocumented. The above code relies upon that. Though, I'd probably stick with a custom implementation like [this one](http://stackoverflow.com/a/28445791/1768303). – noseratio Apr 13 '15 at 19:34
  • 1
    Thanks, I really learned a lot of new things about async/await code from this answer. I elected to choose @Todd Menier's answer because I think he's right that I should just be moving the mail sending code into the form's Show handler. I think yours is a better answer to the (more general) title of the original question, so I've updated the title to limit it to the form issue. Also, thanks for the note about this not actually being async, you're right and using Todd's answer I'll probably just remove the async modifier. – FlintZA Apr 14 '15 at 06:19