Some clarification on the SyncRoot pattern: what is the correct way to use this pattern?

Question

I read something about the SyncRoot pattern as a general rule to avoid deadlocks. And reading a question of a few years ago (see this link), I think I understand that some uses of this pattern may be incorrect. In particular, I focused on the following sentences from this topic:

You’ll notice a SyncRoot property on many of the Collections in System.Collections. In retrospeced, I think this property was a mistake... Rest assured we will not make the same mistake as we build the generic versions of these collections.

In fact, for example, List<T> class doesn't implements SyncRoot property, or more correctly it is implemented explicitly (see this answer), so you must cast to ICollection in order to use it. But this comment argues that making a private SyncRoot field public is as bad practice as locking on this (see this answer), as also confirmed in this comment.

So, if I understand correctly, when I implement a non thread-safe data structure, since it could be used in a multithreaded context, I should not (actually, I must not) provide the SyncRoot property. But I should leave the developer (who will use this data structure) with the task of associating it with a private SyncRoot object as in the following sample code.

public class A
{
    private MyNonThreadSafeDataStructure list;
    private readonly object list_SyncRoot = new object;

    public Method1()
    {
        lock(list_SyncRoot)
        {
            // access to "list" private field
        }
    }

    public Method2()
    {
        lock(list_SyncRoot)
        {
            // access to "list" private field
        }
    }
}

In summary, I understood that the best practices for synchronization/locking should be as follows:

Any private SyncRoot objects should not be exposed through a public property; in other words, a custom data structure should not provide a public SyncRoot property (see also this comment).
In general, it is not mandatory to use private objects for locking (see this answer).
If a class has multiple sets of operations that need to be synchronised, but not with each other, it should have multiple private SyncRoot objects (see this comment).

Is what written above the proper use of this pattern?

My opinion is that if you want to make a class thread-safe internally, you do the locking internally (privately) and document that the class is thread-safe generally. Otherwise, do _no locking_, document that the class is not thread-safe, and let the consumers of your class implement their own locking around it as they need for their specific usage. — Chris Sinclair, Sep 14 '12 at 12:57

score 4 · Accepted Answer · answered Sep 14 '12 at 13:52

I would avoid adding a SyncRoot property to the type that I design, here are the reasons:

Users of my type may need to use different synchronisation mechanism, for example Mutex, or ReaderWriterLock or ReaderWriterLockSlim etc
The type becomes fatter: its responsibility becomes more scattered. Why would I add support for explicit multithreading locking and no support for other fluff? I would force the user to follow only one practise, which may not be the best solution in all cases
I would need to implement the property correctly (no returning this or typeof(MyClass)), i.e. this is wrong:
```
public object SyncRoot {get {return this;}}
```

I would also avoid using SyncRoot property from the .NET framework types. If I need to make a type w/o SyncRoot property threadsafe I will use one locking pattern, and if a type has this property I will still not opt for locking on SyncRoot. This makes my code style consistent and easier to read/maintain.

score 0 · Answer 2 · answered Sep 14 '12 at 13:33

There are a number of concepts here. Firstly what you have implemented correctly is a thread-safe class where no consumers of the class would need to do their own synchronisation. Therefore there is absolutely no need for you to expose the syncRoot object. In the old Collection classes the SyncRoot property was exposed because the classes were not thread-safe.

On locking an arbitrary object and locking your inner collection there is absolutely no difference in terms of correctness or performance of the program. As long as the reference to both does not change they work just as well as parameters to the Monitor.Enter/Exit. Will your inner collection change? If no, mark it as readonly too.

Thirdly there is the comment regarding the use of different locks based on different operations. The classic example of this is the ReaderWriterLock. You should analyse the need to use different levels of locks based on the different functionality exposed by your class.

Some clarification on the SyncRoot pattern: what is the correct way to use this pattern?

2 Answers2

Linked