3
\$\begingroup\$

Seeing as a concurrent collection does not exist in .net that allows specific items to be removed, I have put together the following class.

It's important to note that it is only threadsafe until the LockTimeout on a given operation expires.

The main goal was to to guard against cheeky 'InvalidOperationException: Collection Was Modified' exceptions, which occur if I'm enumerating in one thread, and adding / removing in another.

I've defaulted the LockTimeout to 10 seconds, but in reality, 1 second would still be plenty (in my user case at least).

Lastly, this specific implementation also incorporates INotifyCollectionChanged and INotifyPropertyChanged.

public class ThreadsafeObservableCollection<T> : IList<T>, INotifyCollectionChanged, INotifyPropertyChanged
{
    public event NotifyCollectionChangedEventHandler CollectionChanged;

    public event PropertyChangedEventHandler PropertyChanged;

    private readonly ConcurrentQueue<PropertyChangedEventArgs> _propertyChangedEvents = new ConcurrentQueue<PropertyChangedEventArgs>();

    private readonly ConcurrentQueue<NotifyCollectionChangedEventArgs> _collectionChangedEvents = new ConcurrentQueue<NotifyCollectionChangedEventArgs>();

    private readonly ObservableCollection<T> _collection;

    private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1, 1);

    private static TimeSpan LOCK_TIMEOUT = TimeSpan.FromSeconds(10);

    public ThreadsafeObservableCollection()
    {
        _collection = new ObservableCollection<T>();
        _collection.CollectionChanged += _collection_CollectionChanged;
        (_collection as INotifyPropertyChanged).PropertyChanged += _collection_PropertyChanged;
    }

    private void Wait()
    {
        while (!_semaphore.Wait(LOCK_TIMEOUT))
            _semaphore.Release();
    }

    public void Add(T item)
    {
        Wait();
        try
        {
            _collection.Add(item);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
    }

    public void Clear()
    {
        Wait();
        try
        {
            _collection.Clear();
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
    }

    public bool Contains(T item)
    {
        Wait();
        bool result;
        try
        {
            result = _collection.Contains(item);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
        return result;
    }

    public int Count
    {
        get
        {
            Wait();
            int count;
            try
            {
                count = _collection.Count;
            }
            finally
            {
                _semaphore.Release();
            }
            FireOutstandingEvents();
            return count;
        }
    }

    public bool IsReadOnly => false;

    public T this[int index]
    {
        get
        {
            Wait();
            T item;
            try
            {
                item = _collection[index];
            }
            finally
            {
                _semaphore.Release();
            }
            FireOutstandingEvents();
            return item;
        }
        set
        {
            Wait();
            try
            {
                _collection[index] = value;
            }
            finally
            {
                _semaphore.Release();
            }
            FireOutstandingEvents();
        }
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        Wait();
        try
        {
            _collection.CopyTo(array, arrayIndex);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
    }

    public int IndexOf(T item)
    {
        Wait();
        int index;
        try
        {
            index = _collection.IndexOf(item);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
        return index;
    }

    public void Insert(int index, T item)
    {
        Wait();
        try
        {
            _collection.Insert(index, item);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
    }

    public bool Remove(T item)
    {
        Wait();
        bool result;
        try
        {
            result = _collection.Remove(item);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
        return result;
    }

    public void RemoveAt(int index)
    {
        Wait();
        try
        {
            _collection.RemoveAt(index);
        }
        finally
        {
            _semaphore.Release();
        }
        FireOutstandingEvents();
    }

    private void FireOutstandingEvents()
    {
        while (_propertyChangedEvents.TryDequeue(out PropertyChangedEventArgs arg))
            PropertyChanged?.Invoke(this, arg);
        while (_collectionChangedEvents.TryDequeue(out NotifyCollectionChangedEventArgs arg))
            CollectionChanged?.Invoke(this, arg);
    }

    private void _collection_PropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        _propertyChangedEvents.Enqueue(e);
    }

    private void _collection_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        _collectionChangedEvents.Enqueue(e);
    }

    public IEnumerator<T> GetEnumerator()
    {
        Wait();
        try
        {
            return ((IEnumerable<T>)_collection.ToArray()).GetEnumerator();
        }
        finally
        {
            _semaphore.Release();
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        Wait();
        try
        {
            return _collection.ToArray().GetEnumerator();
        }
        finally
        {
            _semaphore.Release();
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ I like this one. The Thread-safety here achieved by pushing each modification call to UI Thread. The only restriction here: you must construct the instance in the UI Thread. It's easy as it could be. I checked, it's totally thread-safe. I'm using it in almost all of my WPF projects. \$\endgroup\$ – aepot Nov 1 '20 at 20:01
  • \$\begingroup\$ Also your implementation contains a problem. To interact with UI Controls properly you must fire the events in the UI Thread. Otherwise you may probably break the state of the Items Control by Race Condition at CollectionChanged calls e.g. new fire before previous was handled by UI. \$\endgroup\$ – aepot Nov 1 '20 at 20:09
  • \$\begingroup\$ Thus the solution in review is ok to eleminate Collection was modified but not ready to use with UI Data Bindings. The solution in the first comment doesn't fix this exception but fix another one Operation not supported when modification call performed from some pooled thread. One more question: why SemaphoreSlim instead of lock? \$\endgroup\$ – aepot Nov 1 '20 at 20:25
3
\$\begingroup\$

The Wait method is not correct. If the timeout expires the semaphore is released without being taken. This may allow code to run immediately afterwards because there's a slot available (not really as it was already taken but erroneously released) but in the future when Release is called it will fail because the semaphore has already reached its maximum size.

private void Wait()
{
    while (!_semaphore.Wait(LOCK_TIMEOUT))
        _semaphore.Release();
}

Every successful call to Wait must be paired with a call to Release. A Wait returning false is not successful.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.