11

I'm trying to understand what is the best approach to use when calling an async method that updates my ViewModel. Right now, let's say I have something like this:

View:

private async void NavigationHelper_LoadState(object sender, LoadStateEventArgs e)
{
    //Call my ViewModel method to update the data the UI is bound to          
}

ViewModel:

public async Task loadData()
{
    this.Source = await loadStuffFromDatabaseAsync();
}

Now, I'm not sure which one of the following approaches should I use:

1) In my LoadState method, use:

await Task.Run(async () => { await ViewMode.loadData(); });

2) Use Task.Run without awaiting the loadData method inside the Action :

await Task.Run(() => { ViewModel.loadData(); });

3) Call my loadData method with:

await ViewModel.loadData().ConfigureAwait(false);

4) Call the loadData method without awaiting it in my View class and use Task.Run inside my loadData method:

View:

private void NavigationHelper_LoadState(object sender, LoadStateEventArgs e)
{
    ViewModel.loadData();  
}

ViewModel:

public async void loadData()
{
    await Task.Run(async () => 
    {
        this.Source = await loadStuffFromDatabaseAsync();
    });
}

What are the main differences between these approaces?

Is one more efficient that the other, and should I pick one in particular?

Thanks for your help! :)

Sergio

Sergio0694
  • 4,295
  • 3
  • 26
  • 51
  • async void methods should be avoided, and you should follow the naming convention of appending Async to all async methods. – Philip Stuyck Feb 23 '15 at 20:35
  • I believe that Reed Cosby has taught us all that all we need now since Asynch support was integrated into base code is two words Async and Await. We no longer need to call the Task methods. – JWP Feb 23 '15 at 20:51
  • 2
    @JohnPeters: That's not true. There might be code that's blocking the UI thread. – Fred Feb 23 '15 at 21:25
  • @Fred Yup, exactly :) Since my loadData method reads stuff from a database and returns a list of classes, it doesn't need to run on the UI thread, and I was wondering which was the best way to make it run on the background thread. – Sergio0694 Feb 23 '15 at 21:26
  • I don't think any of the code makes sense, here's why...If you are already using Asynch and Await, which you are in the first example of the view model, why would you want to spin off a new thread when a new thread gets spun off anyway? It doesn't make sense. loadData is already async. You already have all you need. – JWP Feb 23 '15 at 21:38
  • @JohnPeters No, that "loadDataAsync" is just the name I gave it in the example. In my code I have a method I wrote that does some async stuff without calling Task.Run or anything else, it just uses the await operator. So from what I understood, it should still run on the caller thread. Shouldn't I move the whole thing to the background thread to be sure the UI doesn't lag? – Sergio0694 Feb 23 '15 at 21:44
  • @PhilipStuyck besides top-level event handlers as in his example – fex Feb 23 '15 at 21:50
  • async void loadData is not an eventhandler in the viewmodel of the last example. – Philip Stuyck Feb 24 '15 at 04:57
  • @Sergio0964, I must have been sleeping yesterday when I read this, my apologies. – JWP Feb 24 '15 at 14:46
  • @JohnPeters No problem man :) – Sergio0694 Feb 24 '15 at 18:46

1 Answers1

3

You should only use Task.Run if you have CPU-bound or blocking work that you want to move off the UI thread. That's not the case here, so the direct call (option 3) is the most natural.

Taking them in turn:

await Task.Run(async () => { await ViewMode.loadData(); });

This option will execute loadData on a thread pool thread. This may not work very well, since loadData is updating the UI (indirectly by setting a VM property). Even if it does happen to work (i.e., some MVVM frameworks can properly handle updates from background threads in some scenarios), it's probably unnecessary since loadData is an asynchronous method.

Furthermore, it adds async state machine overhead for no reason.

await Task.Run(() => { ViewModel.loadData(); });

This option has all the same problems, except it's slightly more efficient since it doesn't have the async state machine overhead. But it's still updating VM properties on a background thread and using a background thread unnecessarily.

public async void loadData()

This one's the worst of all. It inherits the same problems of the others: updating VM properties on a background thread and using an unnecessary background thread. To that it adds the problems of async void. One problem is that NavigationHelper_LoadState cannot catch any exceptions from loadData. Another problem is that loadData is not easily testable.

So just use the simple approach and call it directly:

await ViewModel.loadData().ConfigureAwait(false);
Ortund
  • 1
  • 18
  • 66
  • 133
Stephen Cleary
  • 406,130
  • 70
  • 637
  • 767
  • Thanks for your answer! I just have a doubt: you said I should use Task.Run for CPU-bound or blocking work only: why doesn't loading data from a database count as a blocking work? As it is an IO operation, couldn't that take a long time to complete, and therefore block the calling method that's awaiting that Task? – Sergio0694 Feb 23 '15 at 22:40
  • 1
    The thread is not blocked because it's an asynchronous operation. – Stephen Cleary Feb 23 '15 at 23:15
  • You're right, by "blocking work" you meant some synchronous code to run. Thanks again for your help :) – Sergio0694 Feb 23 '15 at 23:19