Winforms await async/await Task never returns

Question

I have a Winforms exe and from a menu I launch a slow-running process as a Task. It should take about 30 seconds to get the data and then show a dialog. Usually it no longer returns. I catch exceptions and nothing appears in the log so I know it's run ok. The form just never appears, and no CPU time seems to be taking up. Yet I run it in the debugger and step through the code and it works fine. Occasionally it does seem to work on a faster PC. What is happening?

    private async void inPlayRecordToolStripMenuItem_Click(object sender, EventArgs e)
    {
        if (!GetClient()) return;
        {
            await Task.Run(() =>
            {
                LaunchForm();
            });
        }
    }

    private async void LaunchForm()
    {
        try
        {
            {
                var inPlayView = new InPlayView();
                await inPlayView.GetData();
                inPlayView.ShowDialog();
            }
        }
        catch (Exception ex)
        {
            Logger.LogMessage(ex.ToString());
        }
    }

Async methods should typically return a `Task`. See [this answer](https://stackoverflow.com/questions/12144077/async-await-when-to-return-a-task-vs-void) for more info. — , Jan 25 '17 at 21:22
Task.Run is going to execute your code on a background thread from which you should not be accessing UI objects or creating dialogs. — RogerN, Jan 25 '17 at 21:25
Have you tried setting a breakpoint? Are you getting any error messages? (I'd expect an `InvalidOperationException` because of cross-thread issues.) Can you show the code of `GetData()`? — Cameron, Jan 25 '17 at 21:38
@Amy I am not awaiting LaunchForm though, if I make that return a Task I then have to await that as well: await Task.Run(async () => { await LaunchForm (); }); — Rob Sedgwick, Jan 25 '17 at 22:06
@RogerN had the answer I think. The first line of the GetData method had this in: MessageBox.Show(). Removing that seems to do the trick! It obviously works in the debugger where you can get away with it. — Rob Sedgwick, Jan 25 '17 at 22:07

score 1 · Answer 1 · answered Jan 25 '17 at 21:30

1

Do this instead:

private async void inPlayRecordToolStripMenuItem_Click(object sender, EventArgs e)
{
    if (!GetClient()) return;

    await LaunchForm();
}

private async Task LaunchForm()
{
    try
    {
        var inPlayView = new InPlayView();
        await inPlayView.GetData();
        inPlayView.ShowDialog();
    }
    catch (Exception ex)
    {
        Logger.LogMessage(ex.ToString());
    }
}

You don't want Task.Run() for an already async method, and as a general rule, async void is okay for event handlers only, so not the LaunchForm() method.

Also as a comment points out, Task.Run() queues the task to the ThreadPool, so it will end up off the UI thread.

answered Jan 25 '17 at 21:30

sellotape

8,034
2
26
30

1

While this is better practice, this doesn't explain the OP's problem, nor would changing this fix it. – Servy Jan 25 '17 at 21:34
@Servy - you're correct that it doesn't seem to explain why the original code works in the debugger or sometimes; I'd expect it should never work. Perhaps the OP can try my version and tell us how it then behaves. – sellotape Jan 25 '17 at 21:48
Thanks it doesn't seem to make any difference. It runs in the UI thread which makes my exe non-responsive. I want it to respond to mouse clicks while this runs in the background. I should say I have several other forms which have very similar code, they all work fine, the only difference is they are a bit quicker. – Rob Sedgwick Jan 25 '17 at 22:01
1

@RobSedgwick - the UI shouldn't be unresponsive like that. Show us the code for `InPlayView.GetData()` then. I'm assuming `GetClient()` and the constructor for `InPlayView()` are both very quick? – sellotape Jan 25 '17 at 22:04
internal async Task GetData() { await GetInPlays(); await GetEventSummary(); var summary = DataRepository.GetEventSummary(); Data = summary; } – Rob Sedgwick Jan 25 '17 at 22:16
Any long-running sync methods called within your event handler can cause the UI to be unresponsive while those methods execute, if the UI thread is being used to execute those methods. If all your methods call truly async methods only, all the way down (plus only very quick sync methods), your UI should remain responsive. So async calls to database servers, web services, other I/O will be fine; if you have any longer-running compute-bound calls you should offload those by wrapping them in `await Task.Run()`. What you should not need to do is use `Task.Run()` for already-async methods. – sellotape Jan 26 '17 at 08:54

score 0 · Answer 2 · answered Jan 25 '17 at 23:25

I have used async/await on one of my projects and I can't think of a reason to do a ShowDialog in a task. Not sure if this will work but you may want to change your flow a little bit. This should make it more consistent and possibly easier to debug.

private async void inPlayRecordToolStripMenuItem_Click(object sender, EventArgs e) {
    if (!GetClient()) {
        return;
    }

    var playView = await LaunchForm();

    if (playView != null) {
        playView.ShowDialog();
    }
}

private async Task<InPlayView> LaunchForm() {
    try {
        var inPlayView = new InPlayView();
        await inPlayView.GetData();

        return inPlayView;
    } catch (Exception ex) {
        // do cleanup of view if needed
        Logger.LogMessage(ex.ToString());
        return null;
    }
}

Isn't the ShowDialog going to tie up the UI thread? I do the ShowDialog on the tasks, otherwise it won't show. Shouldn't it be Show on the UI thread? — Rob Sedgwick, Jan 26 '17 at 09:42
If by 'tie up' you mean, block the user input, then yes. ShowDialog is suppose to block input for its parent window (since it is now the top most window). If you don't want that, then use Show. — Sean, Jan 27 '17 at 15:14

Winforms await async/await Task never returns

2 Answers2