5

We have an application built according to the MVVM pattern. At various times we kick off tasks to go to a database to retrieve data, we then populate an ObservableCollection to which a WPF control is bound with that data.

We are a little confused, when we populate the ObservableCollection we are doing so on the task thread, not the UI thread, yet the UI is still updated/behaving correctly. We were expecting an error and to have to change the code to populate the collection on the UI thread.

Is this a dangerous scenario and should we populate on the UI thread anyway?

Code to get data:

Task.Factory.StartNew(() =>
    UiDataProvider.RefreshForwardContractReport(fcrIdField.Value)
)
.ContinueWith(task => {
    if (task.Result.OperationSuccess)
    {
        // This updates the ObseravableCollection, should it be run on UI thread??
        RefreshReport(task.Result.OperationResult);
    }
});
Marcos Dimitrio
  • 6,651
  • 5
  • 38
  • 62
Chris
  • 318
  • 2
  • 5
  • 17
  • If the data retrieval from DB is taking time than you should not do that on UI thread as it will hault or freeze the screen which is genreally not a good User-experience.You should do that task on a different thread. – Vishal Jan 30 '14 at 10:22
  • Simple solution. Check the thread you're on. There isn't any need to assume anything. –  Jan 30 '14 at 14:19

8 Answers8

5

WPF keeps allowing more cross-thread operations with each release. Other MVVM platforms do not allow them at all. In the case of ObservableCollection, unless you are using EnableCollectionSynchronization, updating from a background thread is incorrect.

I agree with @avo in that you should treat your ViewModel (your logical UI) as though it had UI thread affinity (like the literal UI). All data binding updates should be done in the UI context.

As @Cameron pointed out, this is most easily done via async:

var result = await Task.Run(
    () => UiDataProvider.RefreshForwardContractReport(fcrIdField.Value));
if (result.OperationSuccess)
{
  RefreshReport(result.OperationResult);
}
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
4

There could be any number of reasons why the continuation is running on the UI thread. The MVVM framework could be helping, or something else is making it run on the UI thread, or you're just getting lucky.

To ensure the continuation runs on the UI thread you can capture the UI TaskScheduler right before like so.

var uiScheduler = TaskScheduler.FromCurrentSyncronizationContext();

Task.Factory.StartNew(() =>
    UiDataProvider.RefreshForwardContractReport(fcrIdField.Value),
    TaskCreationOptions.LongRunning
) // Ensures the task runs in a new thread
.ContinueWith(task => {
    if (task.Result.OperationSuccess)
    {
        RefreshReport(task.Result.OperationResult);
    }
}, uiScheduler); // Runs the continuation on the UI thread.

This assumes that the outer method is run from the UI to begin with. Otherwise you could capture the UI scheduler at a top level and access it globally in your app.

If you can use async/await then the code becomes much easier.

var result = await Task.Factory.StartNew(
    () => UiDataProvider.RefreshForwardContractReport(fcrIdField.Value),
    TaskCreationOptions.LongRunning
);

if (result.OperationSuccess)
{
    RefreshReport(result.OperationResult);
}
Marcos Dimitrio
  • 6,651
  • 5
  • 38
  • 62
Cameron MacFarland
  • 70,676
  • 20
  • 104
  • 133
4

It happens because WPF automatically dispatches the PropertyChanged event to the main thread, unlike all other XAML frameworks. In all other frameworks, a dispatching solution is needed. The best article I've read about MVVM and multi threading https://msdn.microsoft.com/en-us/magazine/dn630646.aspx

StepUp
  • 36,391
  • 15
  • 88
  • 148
2

Considering the MVVM scenario, when you are in the ContinueWith part, i agree that you are in the Non-UI thread and you are updating the properties in the ViewModel (which are bound to UI elemnts) and not the UI elements itself.

Try updating the UI elements in the ContinueWith part, say like

.ContinueWith(task => myTextblock.Text = "SomeText")

In that scenario, u'll get the exception that you are expecting.

P.S - "myTextBlock" is a text block in your View.

Mayur Dhingra
  • 1,527
  • 10
  • 27
1

You should use another (not UI) thread to retrieve long data from DB or another source for prevent UI frozen. But when you try to change UI element from not UI thread it may throw some exception. To change UI from not UI thread you should add update task to UI thread:

Deployment.Current.Dispatcher.BeginInvoke(() => 
{
   some udate action here
});  
Roman Izosimov
  • 210
  • 1
  • 9
1

If you only change your date in the ViewModel, there should be no problem. The ui updates will be raised using INotifyPropertyChanged in your view model.

I prefer the writing of async and await instead of continue with. For most collegues it is more readable. But it is only code sugering, so there will be no different to you implementation.

Sukram
  • 456
  • 3
  • 16
0

I believe you should treat your ViewModel in the same way you treat the UI itself. That is, don't modify it directly from a non-UI thread. INotifyPropertyChanged will not automatically do the magic of marshaling from the worker thread to the UI thread, where controls can be updated.

Related questions:

INotifyPropertyChanged with threads

INotifyPropertyChanged causes cross-thread error

Community
  • 1
  • 1
avo
  • 10,101
  • 13
  • 53
  • 81
  • 2
    "INotifyPropertyChanged will not automatically do the magic of marshaling from the worker thread to the UI thread" Actually, a Binding that is watching an INPC property (i.e., not a DependencyProperty) ***will*** marshall all updates onto the UI thread. This was added in 4.0, I believe. Try it yourself. –  Jan 30 '14 at 14:12
0

add an event to your class as a property and use the background thread (dispatcher) to do your treatment. Once it ends,you invoke the event to upDate the window (the UI thread).

You can also use the background worker Their are small threads used to execute code in a small threads and updates UI after finishing .

Med.Amine.Touil
  • 1,225
  • 2
  • 11
  • 15