BindableCollection thread safety issue

[Yoooi0]

There seems to be an issue with thread safety in BindableCollection.

When two threads modify the collection at the same time, one calling ClearItems, the other calling RemoveItem, it can cause OnCollectionChanging to throw an exception.

System.Reflection.TargetInvocationException: "An error occurred while dispatching a call to the UI Thread"

Inner Exception
ArgumentOutOfRangeException: "Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')"

at Stylet.Execute.OnUIThreadSync(Action action)
at Stylet.BindableCollection`1.RemoveItem(Int32 index)
at System.Collections.ObjectModel.Collection`1.Remove(T item)

Example timeline:

• Create BindableCollection with 1 item
• Thread 1 calls collection.ClearItems()
• Thread 1 enters Execute.OnUIThreadSync
• Thread 2 calls collection.Remove(firstItem), this calls RemoveItem with index argument set to 0
• Thread 2 waits for thread 1
• Thread 1 clears the collection by calling base.ClearItems()
• Thread 2 enters Execute.OnUIThreadSync
• Thread 2 throws exception when calling OnCollectionChanging because the item at index 0 does not exist anymore

[canton7]

Thread 2 calls collection.Remove(firstItem)

Neither ObservableCollection<T> nor BindableCollection<T> has a Remove(T item) method?

[Yoooi0]

Minimal repro: https://dotnetfiddle.net/6ha6Ji
Can take a few runs to throw depending on the call order.

[canton7]

D'oh, of course.

I'm sure sure there's much I can do about that. Collection<T>.Remove is non-virtual, so there's no way to fix it. I could shadow it I guess, but that means that things work so long as someone doesn't cast the BindableCollection<T> to any of its base classes / interfaces.

It's not just Clear -- any call which inserts or removes items before the one being removed will cause this.

[canton7]

Opened #318, but I'm not convinced there's a decent way to solve it.

[Yoooi0]

I think unfortunately the only solution is to extend ICollection<T>+IList<T>+IEnumerable<T> directly.
That will be a breaking change so maybe deprecate BindableCollection<T> and reimplement as ConcurrentObservableCollection<T>?
Cant think of a better solution...

[canton7]

To be honest, I'd rather not make a thread-safe version at all: it's thread-safe in a very marginal and expensive way, and people are much better off just making their ViewModel's single-threaded.