synchronized collection thread safety

2
votes

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?

3
c#multithreadingcollectionsthread-safety
If you're using a SynchronizedCollection, it should already be providing the appropriate locking on operations that add/remove from the collection. – Joe
Does that also supports enumerating the collection in another thread while removing items from different thread? Do I need to lock enumeration section? – Amzath
The whole point of "SynchronizedCollection" existing is to provide thread-safe collections, for all operations (which it does by locking on all operations). If you were using something from ICollection<T> 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

3 Answers

8
votes

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();

enter image description here

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.

7
votes

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.

2
votes

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.