SynchronizedObservableCollection: a thread-safe ObservableCollection

Question

After reading many people talk about the need for a SynchronizedObservableCollection and their implementations I decided to roll my own. I wanted to add a AddRange, RemoveRange, and a way to Replace the entire contents without detaching event listeners.

This is supposed to be a thread-safe collection that produces change events.

• Did I succeed?
• Or have I made any fatal mistake?

/// <summary>
/// A <see cref="SynchronizedCollection&lt;T&gt;"/> that implements features of <see cref="ObservableCollection&lt;T&gt;"/>
/// with the ability to gracefully add, remove, and replace all items.
/// </summary>
public class SynchronizedObservableCollection<T> : SynchronizedCollection<T>, INotifyCollectionChanged, INotifyPropertyChanged
{
 internal readonly PropertyChangedEventArgs CountPropertyChanged = new(nameof(Count));
 internal readonly PropertyChangedEventArgs IndexerPropertyChanged = new("Item[]");
 internal readonly NotifyCollectionChangedEventArgs ResetCollectionChanged = new(NotifyCollectionChangedAction.Reset);
 /// <inheritdoc cref="ObservableCollection&lt;T&gt;.CollectionChanged"/>
 public event NotifyCollectionChangedEventHandler? CollectionChanged;
 /// <inheritdoc cref="ObservableCollection&lt;T&gt;.CollectionChanged"/>
 protected event PropertyChangedEventHandler? PropertyChanged;
 /// <inheritdoc />
 public SynchronizedObservableCollection()
 { }
 /// <inheritdoc />
 public SynchronizedObservableCollection(IEnumerable<T> collection) : base(collection) { }
 /// <inheritdoc />
 public SynchronizedObservableCollection(List<T> list) : base(list) { }
 /// <inheritdoc cref="ObservableCollection&lt;T&gt;.CollectionChanged"/>
 event PropertyChangedEventHandler? INotifyPropertyChanged.PropertyChanged
 {
 add => PropertyChanged += value;
 remove => PropertyChanged -= value;
 }
 private void OnCountPropertyChanged() => OnPropertyChanged(CountPropertyChanged);
 private void OnIndexerPropertyChanged() => OnPropertyChanged(IndexerPropertyChanged);
 private void OnCollectionReset() => OnCollectionChanged(ResetCollectionChanged);
 /// <inheritdoc cref="ObservableCollection&lt;T&gt;.CollectionChanged"/>
 protected virtual void OnPropertyChanged(PropertyChangedEventArgs e)
 {
 lock (SyncRoot)
 PropertyChanged?.Invoke(this, e);
 }
 /// <inheritdoc cref="ObservableCollection&lt;T&gt;.CollectionChanged"/>
 protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
 {
 lock (SyncRoot)
 CollectionChanged?.Invoke(this, e);
 }
 /// <inheritdoc cref="Collection{T}.Add"/>
 public new void Add(T value)
 {
 base.Add(value);
 OnCountPropertyChanged();
 OnIndexerPropertyChanged();
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, value));
 }
 /// <inheritdoc cref="Collection{T}.Remove"/>
 public new bool Remove(T value)
 {
 OnCountPropertyChanged();
 OnIndexerPropertyChanged();
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, value));
 return base.Remove(value);
 }
 /// <inheritdoc cref="Collection{T}.Clear"/>
 public new void Clear()
 {
 base.Clear();
 OnCollectionReset();
 }
 /// <inheritdoc cref="List{T}.AddRange"/>
 public virtual void AddRange(IEnumerable<T> range)
 {
 bool updated = false;
 foreach (var item in range)
 {
 base.Add(item);
 updated = true;
 }
 if (updated)
 {
 OnCountPropertyChanged();
 OnIndexerPropertyChanged();
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, range));
 }
 }
 /// <inheritdoc cref="List{T}.RemoveRange"/>
 public virtual bool RemoveRange(IEnumerable<T> collection)
 {
 bool removed = false;
 foreach (T item in collection)
 removed |= base.Remove(item);
 if (removed)
 {
 OnCountPropertyChanged();
 OnIndexerPropertyChanged();
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, collection));
 }
 return removed;
 }
 
 /// <summary>
 /// Allows you to assign a new collection to this object without all the change listeners being destroyed!
 /// </summary>
 /// <param name="range">The new contents of the collection.</param>
 public virtual void Replace(IEnumerable<T> range)
 {
 var newRange = range.ToList();
 if (newRange.Count == 0)
 {
 base.Clear();
 OnCollectionReset();
 return;
 }
 if (newRange.SequenceEqual(Items))
 return;
 base.Clear();
 OnCollectionReset();
 AddRange(newRange);
 }
}

• Peter Csala: Could you please clarify what's the point of the updated and removed variables inside the AddRange and RemoveRange respectively?
• HackSlash: Those are both flags that control if the property changed events will fire.
• Peter Csala: In case of updated it is used to check whether the IEnumerable parameter has at least one element in it or not. IMHO the if(updated) could be safely replaced with if(range.Any()).
• HackSlash: I didn't do that to avoid multiple enumeration. Maybe that's a premature optimization.
• Peter Csala: Any is implemented in a pretty smart way. First it checks whether it is an ICollection{<TSource>} to use Count property or IIListProvider to use GetCount method. Second it uses an IEnumerator<TSource> to call the MoveNext()` and return its result.

Answer 1

It seems like the Replace has a race condition. Imagine the following situation:

• thread 1 is calling the Replace with an empty array
• thread 2 is calling the Replace with a range which sequentially equivalent with Items.

Scenario #1

1. It might happen that thread2 passed the newRange.SequenceEqual(Items) condition
2. then thread1 clears the collection base.Clear().
3. thread2 returns when the collection is empty.

Scenario #2

1. It might happen that thread1 clears the collection base.Clear().
2. then thread2 do not pass the newRange.SequenceEqual(Items) condition
3. so thread2 clears (the already emptied) collection (again)
4. and finally thread2 adds back all the items

---

So, depending on the execution order either you clear the collection or recreate the collection.

• HackSlash: Thanks! I was largely relying on the locking behavior of base but what you are showing is that I also need to implement more lock (SyncRoot) around my custom methods to make them more thread safe. The trouble is that I can't wrap the whole thing in a lock because nested locks would dead-lock...
• Peter Csala: @HackSlash Exactly, the SynchronizedCollection works fine for primitive (atomic) operations. But whenever you start to combine multiple ones or implement "complex" logic like your Replace it fails. Here you can read more about this topic.