synchronized collection thread safety

Question

I have System.Collections.Generic.SynchronizedCollection shared collection. Our code uses .Net 4.0 Task library to span threads and pass the synchronized collection to the thread. So far threads has not been adding or removing items into the collection. But the new requirement which requires one of the thread has to remove items from the collection while the other thread just read the collection. Do I need to add lock before removing the items from the Collection? If so, would reader thread be thread safe? Or Suggest best way to get the thread safety?

If you're using a SynchronizedCollection, it should already be providing the appropriate locking on operations that add/remove from the collection. — Joe, Nov 25 '12 at 05:31
Does that also supports enumerating the collection in another thread while removing items from different thread? Do I need to lock enumeration section? — Amzath, Nov 25 '12 at 05:37
The whole point of "SynchronizedCollection" existing is to provide thread-safe collections, for all operations (which it does by `lock`ing on all operations). If you were using something from ICollection then you would potentially need to lock on your own. Note that if you're targeting 4.0 framework or later, you might see performance improvements in using System.Collections.Concurrent... — Joe, Nov 25 '12 at 05:40

Alexander Pacha · Answer 1 · 2016-06-16T13:25:25.097

No it is not fully thread-safe. Try the following in a simple Console-Application and see how it crashes with an exception:

var collection = new SynchronizedCollection<int>();

var n = 0;

Task.Run(
    () =>
        {
            while (true)
            {
                collection.Add(n++);
                Thread.Sleep(5);
            }
        });

Task.Run(
    () =>
        {
            while (true)
            {
                Console.WriteLine("Elements in collection: " + collection.Count);

                var x = 0;
                if (collection.Count % 100 == 0)
                {
                    foreach (var i in collection)
                    {
                        Console.WriteLine("They are: " + i);
                        x++;
                        if (x == 100)
                        {
                            break;
                        }

                    }
                }
            }
        });

Console.ReadKey();

Note, that if you replace the SynchronizedCollection with a ConcurrentBag, you will get thread-safety:

var collection = new ConcurrentBag<int>();

SynchronizedCollection is simply not thread-safe in this application. Use Concurrent Collections instead.

Specifically, this is because SynchronizedCollection.GetEnumerator returns the enumerator of the underlying list: `IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable) this.items).GetEnumerator();`. For SynchronizedCollection to be thread-safe here, it would first need to be with ICollection.CopyTo (using Enumerable's ToList would be prone to fail for the same reason) or the enumeration itself would need to lock on SyncRoot for the duration. — user2864740, Aug 07 '21 at 18:33
That said, switching to a ConcurrentBag is _not_ an immediate replacement, as a ConcurrentBag explicitly claims to be "unordered" on the tin. — user2864740, Aug 07 '21 at 18:40

score 7 · Answer 2 · edited Apr 21 '16 at 10:04

As Alexander already pointed out the SynchronizedCollection is not thread safe for this scenario. The SynchronizedCollection actually wraps a normal generic list and just delegates every call to the underlying list with a lock surrounding the call. This is also done in GetEnumerator. So the getting of the enumerator is synchronized but NOT the actual enumeration.

var collection = new SynchronizedCollection<string>();
collection.Add("Test1");
collection.Add("Test2");
collection.Add("Test3");
collection.Add("Test4");

var enumerator = collection.GetEnumerator();
enumerator.MoveNext();
collection.Add("Test5");
//The next call will throw a InvalidOperationException ("Collection was modified")
enumerator.MoveNext();

When using a foreach an enumerator will be called in this way. So adding a ToArray() before enumerating through this array will not work either as this will first enumerate into this array. This enumeration could be faster when what you are doing inside of your foreach so it could reduce the probability of getting a concurrency issue. As Richard pointed out: for true thread safety go for the System.Collections.Concurrent classes.

You can however use the `SyncRoot` property of the collection before you call `ToArray`. — Felix K., Feb 23 '18 at 11:33

score 2 · Accepted Answer · answered Nov 25 '12 at 05:44

2

Yes, SynchronizedCollection will do the locking for you.

If you have multiple readers and just one writer, you may want to look at using a ReaderWriterLock, instead of SynchronizedCollection.

Also, if you are .Net 4+ then take a look at System.Collections.Concurrent. These classes have much better performance than SynchronizedCollection.

answered Nov 25 '12 at 05:44

Richard Schneider

34,944
9
57
73

1

Does synchronized collection be thread safe for multiple readers and one writer? – Amzath Nov 25 '12 at 05:52
I think it depends on the case where the collection is used. For example: If you try to find an item in a collection by a specific value (e.g. an item with a property value larger 20), and change that item, you don't have an atomic operation, but need a lock – Offler Jan 18 '13 at 08:19
3

beware that IEnumerable extension methods are not safe (like Max(), Min() etc). using these extension methods will bite you if it is executing in a multithreaded envoment. – rovsen Jan 26 '16 at 10:45
SynchronizedCollection returns the **non-thread safe** enumerator of the backing list: `IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable) this.items).GetEnumerator();`. This results is even a trivial operation like `ToList()` being **not-thread safe**. – user2864740 Aug 07 '21 at 18:33

synchronized collection thread safety

3 Answers3

Linked