UI is blocking with async await methods

Question

I have 2 DataGrid's on a window, when you click a row in the top DataGrid it calls the CurrentChanged event to get data for the bottom DataGrid to show models for that car brand. The UI gets blocked when clicking a row because the method takes a while to finish. How do I get async await to work so the UI doesn't get blocked?

public class MainWindowViewModel
{
    private ObservableCollection<Company> carCollectionOC = new ObservableCollection<Company>();
    private ObservableCollection<Vehicle> vehicleOC = new ObservableCollection<Vehicle>();
    private VehicleService vehicleService;
    public ICollectionView CarCollection { get; set; }
    public ICollectionView VehicleCollection { get; set; }

    public MainWindowViewModel()
    {
        this.carCollectionOC.Add(new Company() { Brand = "Ford", Established = 1903 });
        this.carCollectionOC.Add(new Company() { Brand = "Vauxhall", Established = 1857 });
        this.CarCollection = new ListCollectionView(this.carCollectionOC);
        this.CarCollection.CurrentChanged += CarCollection_CurrentChanged;
        this.vehicleService = new VehicleService();
        this.VehicleCollection = new ListCollectionView(this.vehicleOC);
    }

    private async void CarCollection_CurrentChanged(object sender, EventArgs e)
    {
        Company company = (Company)(sender as ICollectionView).CurrentItem;
        this.vehicleOC = await this.GetVehiclesAsync(company.Brand);
    }

    private async Task<ObservableCollection<Vehicle>> GetVehiclesAsync(string carBrand)
    {
        this.vehicleOC.Clear();
        foreach (var item in await this.vehicleService.GetVehicleListAsync(carBrand))
            this.vehicleOC.Add(item);
        Console.WriteLine("finishing GetVehiclesAsync");
        return this.vehicleOC;

    }
}

The GetVehicleListAsync is:

    public Task<List<Vehicle>> GetVehicleListAsync(string carBrand)
    {
        for (var i = 600000000; i >= 0; i--)
        {
            // simulate a long process
        }
        return Task.FromResult(this.vehicleList.Where(item => item.Brand == carBrand).ToList());
    }

This article might help. https://devblogs.microsoft.com/pfxteam/await-and-ui-and-deadlocks-oh-my/ — Jim Yarbro, Nov 17 '20 at 09:40
Your `GetVehicleListAsync` isn't actually async -- it's doing all of its work on the thread which invokes it, and then using `Task.FromResult` to create an already-completed `Task`. In order to actually be async, `GetVehicleListAsync` needs to return *quickly*, and return a `Task` which hasn't yet completed. Later on when the work is complete, it will complete the `Task`. If your "long process" is CPU-bound (not IO-bound, or using sleepds, etc), then using `Task.Run` is an easy way to move this work to a background thread, and return a `Task` which completes when the operation is complete. — canton7, Nov 17 '20 at 09:40
In short, I think you should probably research the async and await pattern and tasks a little more. You seem to be having problems with the very basics. Where a little resreach at this stage will benifit more than these sorts of questions — TheGeneral, Nov 17 '20 at 09:49

score 4 · Accepted Answer · edited Nov 17 '20 at 10:51

4

GetVehicleListAsync is not an asynchronous method; it is a synchronous method that returns a Task<Vehicle> and will block.

As you aren't doing anything asynchronous i.e. I/O, you should use Task.Run to offload the expensive code to the ThreadPool, which leaves the UI thread unblocked:

private async void CarCollection_CurrentChanged(object sender, EventArgs e)
{
    Company company = (Company)(sender as ICollectionView).CurrentItem;
    this.vehicleOC.Clear();
    foreach (var vehicle in await Task.Run(() => this.GetVehicles(company.Brand)))
        this.vehicleOC.Add(vehicle);
}

private IEnumerable<Vehicle> GetVehicles(string carBrand)
{
    return this.vehicleService.GetVehicleList(carBrand));
    Console.WriteLine("finishing GetVehicles");
}

public List<Vehicle> GetVehicleList(string carBrand)
{
    for (var i = 600000000; i >= 0; i--)
    {
        // simulate a long process
    }
    return this.vehicleList.Where(item => item.Brand == carBrand).ToList();
}

GetVehiclesAsync and GetVehicleListAsync are inherently synchronous and should be implemented as such.

edited Nov 17 '20 at 10:51

Clemens

123,504
12
155
268

answered Nov 17 '20 at 09:41

Johnathan Barclay

18,599
1
22
35

2

This might fail in this specific scenario, because if `ObservableCollection` is bound to UI (and here it is) - adding items there from non-UI thread will fail. – Evk Nov 17 '20 at 09:48
It is debatable whether the `GetVehicleListAsync` is an asynchronous method or not. In my opinion [it is](https://stackoverflow.com/questions/61837259/is-having-a-return-type-of-task-enough-to-make-a-method-run-asynchronously/61837980#61837980), but other opinions exist. – Theodor Zoulias Nov 17 '20 at 09:49
This implementation errors - System.NotSupportedException: 'This type of CollectionView does not support changes to its SourceCollection from a thread different from the Dispatcher thread.' How do I stop this? – chris Nov 17 '20 at 09:54
@TheodorZoulias current implementation just doesn't make sense, so naming doesn't really matter for the broken thing until this is resolved first. – Evk Nov 17 '20 at 09:58
@Evk names do matter. Personally I have quit a long-time job after a naming disagreement. Btw calling the `GetVehicleListAsync` "the broken thing" still qualifies as a name IMHO. – Theodor Zoulias Nov 17 '20 at 10:10
@TheodorZoulias This is a prime example of why IMO a `Task` returning method is not in itself asynchronous. TAP is a leaky abstraction, and the implementation *matters*. If the method is implemented synchronously it blocks, and is therefore not asynchronous. – Johnathan Barclay Nov 17 '20 at 10:11
1

Johnathan I an not a fan of calling the `Dispatcher.Invoke` in a loop, because it could add significant synchronization overhead. And there is no need to do so in this case. You could fetch the data in the background thread, and then do the looping that updates the UI in the UI thread. – Theodor Zoulias Nov 17 '20 at 10:21
@TheodorZoulias what would your implementation look like? – chris Nov 17 '20 at 10:33
2

@chris `var data = await Task.Run(() => this.vehicleService.GetVehicleList(carBrand));`, then use the data to populate the UI. I like this more than using the `Dispatcher`. Stephen Cleary's advice ["Don't Use Task.Run in the Implementation"](https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html) applies mainly to library code. When writing UI-helper methods in the application level it's OK to take shortcuts and use `Task.Run` here and there IMHO. – Theodor Zoulias Nov 17 '20 at 10:49
@TheodorZoulias Thanks. By the way, should I be using Stephens 'NotifyTask' for any of this? I don't know how to implement it if so. There aren't many examples. – chris Nov 17 '20 at 15:49
@chris I have no experience of using the [`NotifyTask`](https://github.com/StephenCleary/Mvvm/blob/master/future/Nito.Mvvm.Async/NotifyTask.cs) class, so I have no advice to offer. AFAIK it may not be a very popular tool. – Theodor Zoulias Nov 17 '20 at 16:18

score 1 · Answer 2 · answered Nov 17 '20 at 09:40

1

Your GetVehicleListAsync method is actually not asynchronous. Change it to:

public Task<List<Vehicle>> GetVehicleListAsync(string carBrand)
{
    return Task.Run(() => 
    {
       for (var i = 600000000; i >= 0; i--)
       {
           // simulate a long process
       }
       return this.vehicleList.Where(item => item.Brand == carBrand).ToList();
    }
}

answered Nov 17 '20 at 09:40

Quercus

2,015
1
12
18

2

I wouldn't use `Task.Run` in this method: https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html – Johnathan Barclay Nov 17 '20 at 09:43

UI is blocking with async await methods

2 Answers2