Mvvmcross: MvxObservableCollection thread unsafety

[slarti-zak]

🐛 Bug Report

The MvxObservableCollection tries to be thread safe by modifying the collection instantly (on the caller thread/task) but delaying the change events and posting them on the main thread using the IMvxMainThreadAsyncDispatcher.

This does not work in cases when the collection is modified and a bound view accesses the collection at the same time. For example: The view getting the count of the collection and then looping over the elements by index to present them. If the collection is modified while the view executes such a loop, an incorrect access may occur. Such an access cannot be synchronized across threads and may result in a crashing app.

I have created an example project here that showcases that problem on android with a MvxRecyclerView. The app spams the collection with modifications from a single background thread. If the user keeps scrolling to the bottom, the app will eventually crash with an out of bounds exception. Other OS have similiar problems.

It may also happen that the view may access the same element multiple times, due to concurrent adds/removals on the collection. This is also shown in the example project, where the same element is shown multiple times in the recycler view.

I'm not sure what the correct way to solve this problem would be. Since MvxObservableCollection extends ObservableCollection the implementation of some methods (single elment Add, Remove) cannot be changed and since ObservableCollection is not thread safe the MvxObservableCollection is not thread safe.

One way could be to remove the thread synchronization code from MvxObservableCollection and leaving it up to the user to access the collection in a thread safe manner (like a List or Dictionary).

Expected behavior

• The MvxObservableCollection should not result in App-Crashes when bound to list views (on any OS).
• Bound views should always display the exact contents of a MvxObservableCollection.

Reproduction steps

Run the sample project here.

Configuration

Version: 6.2.1

Platform:

• [x] :iphone: iOS
• [x] :robot: Android
• [ ] :checkered_flag: WPF
• [ ] :earth_americas: UWP
• [ ] :apple: MacOS
• [ ] :tv: tvOS
• [ ] :monkey: Xamarin.Forms
  Possibly all of them but explicitly tested only on Android and iOS.

[slarti-zak]

I have extended the sample project to iOS, which behaves the same way as Android does. Keep scrolling the list to the bottom and it will crash with a similar error.

[slarti-zak]

I have looked at this further and right now I see two ways to solve this:

1 Synchronize Collection Access

Every access to a Observable Collection has to be synchronized. But it is not enough to only lock each of the methods since one could query the index of an element and then use that as a parameter for another method. It may happen that that index would no longer be valid. So a whole operation on a collection would have to be atomic.

WPF has solved this by setting a special flag which forces the view side to synchronize access to a collection. The user then also has to synchronize his threaded access to the collection. One point of their implementation should be noted as a hint for all implementations:

Ensure that a change to the collection and the notification of that change (through INotifyCollectionChanged) are atomic

This is something the current MvxObservableCollection is not doing.

Basically this means that each adapter (TableViewSources, RecyclerAdapter, ...) on every platform would have to be adapted to include a synchronization mechanism. In addition the user has to be careful with threaded access.

Cons

• Access on the collection has to wait for the other side to finish (service calls waiting for the ui)
• Many changes neccessary since it has to be adapted for every platform
• User has to explicitly pay attention to threaded collection access

2 Copy the collection

I have fiddled with another solution that uses a second (custom) ObservableCollection (I will call it UiObservableCollection). This UiObservableCollection implements the neccessary interfaces of INotifyCollectionChanged, IList, and so on but cannot be modified directly.

Instead it has to be attached to another observable collection and listens to the change events of that collection. It then queues the change events onto the ui thread where they are applied to the collection. This collection is thus a "delayed" view of another collection. Since the UiObservableCollection is only changed on the ui thread, no further synchronization is neccessary.

Basically this means: one "user" collection for the background thread, that gets modified there and one "UI" collection that synchronizes the user collection onto the ui thread.

Maybe this UiObservableCollection could also be automatically wrapped around another ObservableCollection when one is bound to an adapter, making it transparent for the user.

Cons

• The UiObservableCollection is a copy of another collection, increasing memory consumption

Finishing Thoughts

I'm open to other suggestions and would be available to create a pull request once a satisfying solution has been found. Maybe I'm thinking at this too hard?

[picolino]

Hello from 2020. Problem is still exists.

I think it's not so specific case but basic functionality that must be developed inside framework to avoid such hard-to-debug errors

[picolino]

I implemented second solution, which was proposed by @slarti-zak . Works perfect. Thank you for the tip.

public class ThreadSafeObservableCollection<T> : IList<T>, INotifyCollectionChanged, INotifyPropertyChanged, IDisposable
{
    private readonly MvxObservableCollection<T> threadUnsafeObservableCollection;

    private IList<T> items;

    public ThreadSafeObservableCollection() : this(new MvxObservableCollection<T>())
    {
    }

    public ThreadSafeObservableCollection(MvxObservableCollection<T> threadUnsafeObservableCollection)
    {
        this.threadUnsafeObservableCollection = threadUnsafeObservableCollection;
        this.threadUnsafeObservableCollection.CollectionChanged += ThreadUnsafeObservableCollectionChanged;

        items = new List<T>(this.threadUnsafeObservableCollection);
    }

    private void ThreadUnsafeObservableCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        items = new List<T>((IEnumerable<T>) sender);
        CollectionChanged?.Invoke(items, e);
    }

    public event NotifyCollectionChangedEventHandler CollectionChanged;
    public event PropertyChangedEventHandler PropertyChanged;

    public int Count => items.Count;
    public bool IsReadOnly => false;

    public T this[int index]
    {
        get => items[index];
        set => threadUnsafeObservableCollection[index] = value;
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        items.CopyTo(array, arrayIndex);
    }

    #region Reading only

    public IEnumerator<T> GetEnumerator()
    {
        return items.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public bool Contains(T item)
    {
        return items.Contains(item);
    }

    public int IndexOf(T item)
    {
        return items.IndexOf(item);
    }

    #endregion

    #region Modificating only

    public void Add(T item)
    {
        threadUnsafeObservableCollection.Add(item);
    }

    public void Clear()
    {
        threadUnsafeObservableCollection.Clear();
    }

    public bool Remove(T item)
    {
        return threadUnsafeObservableCollection.Remove(item);
    }

    public void Insert(int index, T item)
    {
        threadUnsafeObservableCollection.Insert(index, item);
    }

    public void RemoveAt(int index)
    {
        threadUnsafeObservableCollection.RemoveAt(index);
    }

    public void ReplaceWith(IEnumerable<T> items)
    {
        threadUnsafeObservableCollection.ReplaceWith(items);
    }

    public void AddRange(IEnumerable<T> items)
    {
        threadUnsafeObservableCollection.AddRange(items);
    }

    #endregion

    public void Dispose()
    {
        threadUnsafeObservableCollection.CollectionChanged -= ThreadUnsafeObservableCollectionChanged;
    }
}

[Cheesebaron]

@picolino this is not thread safe, because the underlying collection is not thread safe. You are just enforcing the CollectionChanged event is fired on a specific thread.

We already do this in MvvmCross: https://github.com/MvvmCross/MvvmCross/blob/develop/MvvmCross/ViewModels/MvxObservableCollection.cs#L72

[picolino]

@Cheesebaron , not only CollectionChanged event, but updating copy of initial collection for read operations.

Thank you for sharing that CollectionChanged event raising by MainThread always. We can handle that without IMvxMainThreadAsyncDispatcher, so I changed code above.