When is it acceptable to lock the object I'm using and when should I use a dedicated synclock object

Question

Lets say I have three different classes that each lock a collection in a slightly different way. Class A directly locks the collection, class B locks the collection's SyncRoot property, and class C uses a dedicated object to lock the collection. My understanding is that the convention is to use one of the last two methods, but I'm curious as to when it's acceptable to use the method class A uses and if there are any potential dangers. Also are there any benefits/dangers between the methods used by classes B and C?

public class A
{
    private List<object> _list = new List<object>();

    public void DoStuff()
    {
        lock(_list)
        {
            ...
            ...
        }
    }
}

public class B
{
    private List<object> _list = new List<object();

    public void DoStuff()
    {
        lock(((ICollection)_list).SyncRoot)
        {
            ...
            ...
        }
    }
}

public class C
{
    private List<object> _list = new List<object>();
    private object _listSyncLock = new object();

    public void DoStuff()
    {
        lock(_listSyncLock)
        {
            ...
            ...
        }
    }
}

Answer 1

The idea of creating a private object to lock on is to ensure that your type, and only your type, is locking on that object. If you lock on some object that is available in other classes, then you need to consider what those other classes are doing with that object, and when they are locking on it, when trying to reason about what can possibly happen when your code is executing. If you lock on something like say, this, or a public property/field, then you need to consider the possibility that any code, anywhere in the entire application, could possibly lock on that object. That makes it actually quite hard to reason about the code, to avoid deadlocks, etc.

The issue here is that the list (as far as you have shown) is never exposed outside of this class. If the list really is never exposed elsewhere, then nothing else can lock on it. If nothing else can lock on it, then there is no problem locking on it, and no real reason to create a second object to lock on.

The goal of the SyncLock is to explicitly allow types that haven't otherwise shared an object to lock on besides the list, to lock on that object. If you never expose the list elsewhere, nothing else will be able to grab that sync lock, so it's not really any different than locking on the list directly.

Of course, if you do expose the list externally (even if you cast it to something like an IEnumerable, since that sill results in the same object reference to lock on) then options A and B are pretty much the same, and both should be avoided. The whole purpose of that pattern is to have something to lock on either if your type has no unchanging instance fields to lock on, or where they are exposed publicly. Although in some cases it's done just for the sake of readability, because people are so used to this pattern, and it's not that awful to create the object for just that reason, even though, as mentioned, it may not actually be necessary.

Answer 2

The purpose of SyncRoot is to allow for the possibility that an implementation of IList<T> might be a wrapper on some other object. Given:

var list = new List<String>();
var wrapper = new ReadOnlyCollection<String>(list);
IList ilist1 = list, ilist2 = wrapper;

it is important that no attempt be made to read an item from ilist2 while a change is being made to ilist1. Consequently, both must have the same SyncRoot. To allow for this, the SyncRoot property of ReadOnlyCollection<T> returns the SyncRoot of the wrapped collection. Note that the List<T> syncroot property returns an object created just for that purpose. If it did not, code with a ReadOnlyCollection<String> that wrapped a List<String> could cast its SyncRoot object to List<String> and use it to modify the list.

The only time SyncRoot is really relevant is when some, though not all, references to a collection will be made through wrappers, and the pieces of code which use the collection are not all aware of each others' existence. Note that SyncRoot has been largely deprecated because no consistent pattern of usage emerged, and having such a feature used inconsistently is worse than not having it at all.