UI error display window with task async in WPF

Question

I am new to async and was wondering if any improvement can be made to the solution I put together from scattered information on the web.

The following (working) code shows how a button press runs logic and if there are any errors they will appear in the returned task.Exception.Handle variable which I can check in my ContinueWith code. If it's not null, I open the error dialog window for the user. By passing in FromCurrentSynchronizationContext, I get to open the window on the UI thread and avoid issues with UI thread access.

Any improvements to make it more readable or suggest a different patter for this would be much welcomed.

public void StartButton()
{
    //prevent multiple button presses with IsRunning switch
    if (!IsRunning)
    {
        IsRunning = true;

        Threading.Task.Factory.StartNew(() =>
        {
            ProgramLogic();
            IsFabRunning = false;
        })
        .ContinueWith(p =>
        {
            if (p.Exception != null)
                p.Exception.Handle(x =>
                {
                    logger.Error(p.Exception);
                    windowManager.ShowDialog(new ForceCloseViewModel(p.Exception.Message), null, null);
                    IsFabRunning = false;
                    return true;
                });
        }, Threading.TaskScheduler.FromCurrentSynchronizationContext());
    }
}

Answer 1

This is a good fit for async await:

public async void RunProgramLogic()
{
    if (!IsRunning)
    {
        IsRunning = true;
        try
        {
            await Task.Run(() => ProgramLogic())
                      .ConfigureAwait(true); // schedules back to the synchronization context the method was called from.
        }
        catch (Exception e)
        {
            logger.Error(e); // Log or LogError feels like better names
            windowManager.ShowDialog(new ForceCloseViewModel(e.Message), null, null);
        }
        finally
        {
            IsRunning = false;
        }
    }
}

Regarding async await As you can see using async and await removes quite a bit of noise from the code. In general async void is smelly and should only be used for eventhavndlers. In your case it feels like a decent fit since you handle the exceptions. Async void means it cannot be awaited.

Regarding command: While I agree that this looks like a fit for a RelayCommand it is not trivial due to the async.

Here is a good read on commands for async scenarios.

Most RelayCommand implementations handle raising CanExecuteChanged poorly by calling CommandManager.InvalidateRequerySuggested which is very inefficient.

Here is a nice implementation.

Regarding scheduling Always be explicit with .ConfigureAwait(true|false) when awaiting. The default is true which means that the code continues on the synchronizationcontext it came from. This is a strange default in my opinion and ConfigureAwait is a poorly named method. In your case I think the logger and windowManager should be thread agnostic enabling .ConfigureAwait(false) Suggested implementation:

public class WindowManager
{
    public void ShowDialog(object viewModel, ...)
    {
        if (Dispatcher.CurrentDispatcher.CheckAccess())
        {
            ShowDialogCore(viewModel, ...);
        }
        else
        {
            Dispatcher.CurrentDispatcher.BeginInvoke(new Action(() => ShowDialogCore(viewModel, ...)));
        }
    }

    private void ShowDialogCore(object viewModel, ...)
    {
        ...
    }
}

The upside with implementing it like this is that the caller will not need to remember to call it on the UI thread. It is the job of the windowmanager to schedule if needed. Hence fixing many potential bugs in one place.

Regarding the pattern Using a flag like this is fine and will work well if you refactor to relaycommand. A try-finally block makes it clear in my opinion.

Answer 2

This problem:

public void StartButton()
{
    //prevent multiple button presses with IsRunning switch
    if (!IsRunning)
    {
        IsRunning = true;

... can be solved far more elegantly by implementing MVVM and using commands, both of which you should be doing anyway. I'd write an example using your code, but you've not given us much to work with (and I don't have the time right now anyway).

Also, StartButton() is a really bad name for a method.