C# Best practice of thread-saftey when working with collections (that aren't Concurrent already)

Question

My question is more like something I want to confirm with other people so I'm sure I got the right answer. When you want to make a non thread-safe operation such as enumerating over a collection, the best thing to do would be to call the Synchronized method if it exists for that collection.

e.g: var syncAL = ArrayList.Synchronized( myAL );

and then you can do something like with no worries:

foreach (var item in syncAL){
  // Do something
}

Now I personally don't really use ArrayLists and it was just for the example, but for a regular List the Synchronized method doesn't exist so here the best practice would be:

lock (list.SyncRoot){
 // non thread-safe operation, e.g:
 list.ForEach(item => Console.WriteLine(item.ToString()));
}

because of course you can create your own arbitrary object, but if you're using some collection object in more than one class, you'll want to make sure you're using the same object for locking in all of those classes and this seems like a pretty comfortable way of doing that.

I want to make sure it is really the best practice and there isn't anything I'm missing before I'll add what I wrote as a development tip for my team :)

The source of my understanding of this: MSDN SyncRoot

Answer 1

and then you can do something like with no worries:

foreach (var item in syncAL){
  // Do something
}

No. From MSDN :

Enumerating through a collection is intrinsically not a thread-safe procedure. Even when a collection is synchronized,

And to the core question:

I want to make sure it is really the best practice

No, it is an outdated practice. Notice that SyncRoot is limited to ICollection and that it is not supported for ICollection<T>. Newer collections (List<>) do not support it, that is your main clue.

Answer 2

Wrapping collection in ArrayList.Synchronized wrapper doesn't make enumeration thread safe, MSDN explicitly states this:

...Even when a collection is synchronized, other threads can still modify the collection, which causes the enumerator to throw an exception.

So best approach would still be locking on SyncRoot, or using thread-safe collections from System.Collections.Concurrent namespace.

Internal implementation of ArrayList.Synchronized will just create instance of internal class SyncArrayList, which will lock on SyncRoot for any operation, like this:

  public override int Add(object value)
  {
    lock (this._root)
      return this._list.Add(value);
  }