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?

Answer 1

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.

Answer 2

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.