MVVM async await pattern

Question

I've been trying to write an MVVM screen for a WPF application, using the async & await keywords to write asynchronous methods for 1. Initially loading data, 2. Refreshing the data, 3. Saving changes and then refreshing. Although I have this working, the code is very messy and I can't help thinking that there must be a better implementation. Can anyone advise on a simpler implementation?

This is a cut-down version of my ViewModel:

public class ScenariosViewModel : BindableBase
{
    public ScenariosViewModel()
    {
        SaveCommand = new DelegateCommand(async () => await SaveAsync());
        RefreshCommand = new DelegateCommand(async () => await LoadDataAsync());
    }

    public async Task LoadDataAsync()
    {
        IsLoading = true; //synchronously set the busy indicator flag
        await Task.Run(() => Scenarios = _service.AllScenarios())
            .ContinueWith(t =>
            {
                IsLoading = false;
                if (t.Exception != null)
                {
                    throw t.Exception; //Allow exception to be caught on Application_UnhandledException
                }
            });
    }

    public ICommand SaveCommand { get; set; }
    private async Task SaveAsync()
    {
        IsLoading = true; //synchronously set the busy indicator flag
        await Task.Run(() =>
        {
            _service.Save(_selectedScenario);
            LoadDataAsync(); // here we get compiler warnings because not called with await
        }).ContinueWith(t =>
        {
            if (t.Exception != null)
            {
                throw t.Exception;
            }
        });
    }
}

IsLoading is exposed to the view where it is bound to a busy indicator.

LoadDataAsync is called by the navigation framework when the screen is first viewed, or when a refresh button is pressed. This method should synchronously set IsLoading, then return control to the UI thread until the service has returned the data. Finally throwing any exceptions so they can be caught by the global exception handler (not up for discussion!).

SaveAync is called by a button, passing updated values from a form to the service. It should synchronously set IsLoading, asynchronously call the Save method on the service and then trigger a refresh.

Answer 1

There are a few problems in the code that jump out to me:

• Usage of ContinueWith. ContinueWith is a dangerous API (it has a surprising default value for its TaskScheduler, so it should really only be used if you specify a TaskScheduler). It's also just plain awkward compared to the equivalent await code.
• Setting Scenarios from a thread pool thread. I always follow the guideline in my code that data-bound VM properties are treated as part of the UI and must only be accessed from the UI thread. There are exceptions to this rule (particularly on WPF), but they're not the same on every MVVM platform (and are a questionable design to begin with, IMO), so I just treat VMs as part of the UI layer.
• Where the exceptions are thrown. According to the comment, you want exceptions raised to Application.UnhandledException, but I don't think this code will do that. Assuming TaskScheduler.Current is null at the start of LoadDataAsync/SaveAsync, then the re-raising exception code will actually raise the exception on a thread pool thread, not the UI thread, thus sending it to AppDomain.UnhandledException rather than Application.UnhandledException.
• How the exceptions are re-thrown. You'll lose your stack trace.
• Calling LoadDataAsync without an await. With this simplified code, it'll probably work, but it does introduce the possibility of ignoring unhandled exceptions. In particular, if any of the synchronous part of LoadDataAsync throws, then that exception would be silently ignored.

Instead of messing around with the manual-exception-rethrows, I recommend just using the more natural approach of exception propagation through await:

• If an asynchronous operation fails, the task gets an exception placed on it.
• await will examine this exception, and re-raise it in a proper way (preserving the original stack trace).
• async void methods do not have a task on which to place an exception, so they will re-raise it directly on their SynchronizationContext. In this case, since your async void methods run on the UI thread, the exception will be sent to Application.UnhandledException.

(the async void methods I'm referring to are the async delegates passed to DelegateCommand).

The code now becomes:

public class ScenariosViewModel : BindableBase
{
  public ScenariosViewModel()
  {
    SaveCommand = new DelegateCommand(async () => await SaveAsync());
    RefreshCommand = new DelegateCommand(async () => await LoadDataAsync());
  }

  public async Task LoadDataAsync()
  {
    IsLoading = true;
    try
    {
      Scenarios = await Task.Run(() => _service.AllScenarios());
    }
    finally
    {
      IsLoading = false;
    }
  }

  private async Task SaveAsync()
  {
    IsLoading = true;
    await Task.Run(() => _service.Save(_selectedScenario));
    await LoadDataAsync();
  }
}

Now all the problems have been resolved:

• ContinueWith has been replaced with the more appropriate await.
• Scenarios is set from the UI thread.
• All exceptions are propagated to Application.UnhandledException rather than AppDomain.UnhandledException.
• Exceptions maintain their original stack trace.
• There are no un-await-ed tasks, so all exceptions will be observed some way or another.

And the code is cleaner, too. IMO. :)