Thread safety with Rx and ObservableCollection

Question

In a pursuit for more responsive way to update ListBox with large number of items I turned to Rx. This is my implementation of it:

    ObservableCollection<FileData> _fileCollection = new ObservableCollection<FileData>();
    public ObservableCollection<FileData> FileCollection { get { return _fileCollection; } }
    public static object _fileCollectionLock = new object();

    public Subject<FileData> subject = new Subject<FileData>();

    public MainWindow( )
    {
        InitializeComponent();
        BindingOperations.EnableCollectionSynchronization(_fileCollection, _fileCollectionLock);
        UpdateFileList(subject);
    }

    private void UpdateFileList(IObservable<FileData> sequence)
    {
        sequence.Subscribe(value=>_fileCollection.Add(value));
    }

    private void ListFiles(string fullpath)
    { 
        _fileCollection.Clear(); //crashed once
        Task.Factory.StartNew(() =>
        {
            DirectoryInfo info = new DirectoryInfo(fullpath);
            IEnumerable files = info.EnumerateFiles(Filter + "*", SearchOption.TopDirectoryOnly,true);
            foreach (FileInfo file in files)
            {
                ...
                FileData fd = new FileData(filename, filedate, filesize, fileext);      
                subject.OnNext(fd);

It happened once that in my code crashed on _fileCollection.Clear(); (Forgot the error, sorry). Do I need to use locks and where?

Answer 1

There are several problems with your implementation, rooted in a misunderstanding of how to correctly use EnableCollectionSynchronization. To be honest though, I am not surprised - it is poorly documented and the design is not great (I suspect mainly due to having to make it work with WPF without breaking changes).

Correct usage of EnableCollectionSynchronization

I will briefly cover the correct usage:

EnableCollectionSynchronization registers a lock that WPF will use whenever it needs to access the collection (e.g. when controls enumerate it).

In .NET 4.5, collection change events raised on background threads (i.e. threads that are not the thread from where EnableCollectionSynchronization was called) are queued and marshalled to the UI thread anyway.

Importantly, when using this method you must still lock any access to the collection yourself using the same lock you registered with EnableCollectionSynchronization. In particular, there is nothing in your code preventing Clear() running concurrently with Add() and this is most certainly the cause of the exception you are seeing.

A good place to call EnableCollectionSynchronization is from a handler to the BindingOperations.CollectionRegistering event, which guarantees the call is made before any CollectionView instances are created.

Best Practice for updating UI bound collections

All that said, I believe that you should abandon this approach entirely - it is far better to instead undertake to only update UI bound collections on the UI thread. RX is great for this, you can use ObserveOn (see here for a good primer) to marshall updates to the UI thread. If you do this, you no longer need to worry about synchronizing access to the collection and your life will be much simpler.

My advice - do all the work of figuring out what updates are required on a background thread, but modify the collection on the UI thread itself. This approach is almost always fast enough, and if it isn't you probably need to think about your design.

Have at look at my answer here for a discussion and links to articles on updating on the UI.

Also have a look at this question for another discussion/analysis of issues with large numbers of changes happening on the UI thread, and how you can buffer huge numbers of changes to avoid hogging the dispatcher.

It's also worth looking at the ReactiveUI framework. See also here.