What's the use of the SyncRoot pattern?

Question

I'm reading a c# book that describes the SyncRoot pattern. It shows

void doThis()
{
    lock(this){ ... }
}

void doThat()
{
    lock(this){ ... }
}

and compares to the SyncRoot pattern:

object syncRoot = new object();

void doThis()
{
    lock(syncRoot ){ ... }
}

void doThat()
{
    lock(syncRoot){ ... }
}

However, I don't really understand the difference here; it seems that in both cases both methods can only be accessed by one thread at a time.

The book describes ... because the object of the instance can also be used for synchronized access from the outside and you can't control this form the class itself, you can use the SyncRoot pattern Eh? 'object of the instance'?

Can anyone tell me the difference between the two approaches above?

Thanks in advance

Answer 1

If you have an internal data structure that you want to prevent simultaneous access to by multiple threads, you should always make sure the object you're locking on is not public.

The reasoning behind this is that a public object can be locked by anyone, and thus you can create deadlocks because you're not in total control of the locking pattern.

This means that locking on this is not an option, since anyone can lock on that object. Likewise, you should not lock on something you expose to the outside world.

Which means that the best solution is to use an internal object, and thus the tip is to just use Object.

Locking data structures is something you really need to have full control over, otherwise you risk setting up a scenario for deadlocking, which can be very problematic to handle.

Answer 2

Here is an example :

class ILockMySelf
{
    void doThat()
    {
        lock(this){ ... }
    }
}

class WeveGotAProblem
{
    ILockMySelf anObjectIShouldntUseToLock;

    void doThis()
    {
        lock(anObjectIShouldntUseToLock)
        {
            // Following line should run in a separate thread
            // for the deadlock to happen
            anObjectIShouldntUseToLock.doThat();
        }
    }
}

You see that the second class can use an instance of the first one in a lock statement. This leads to a deadlock in the example.

The correct SyncRoot implementation is:

object syncRoot = new object();

void doThis()
{
    lock(syncRoot ){ ... }
}

void doThat()
{
    lock(syncRoot ){ ... }
}

as syncRoot is a private field, you don't have to worry about external use of this object.

Answer 3

See this Jeff Richter's article. More specifically, this:

using System;
using System.Threading;

class App {
   static void Main() {
      // Construct an instance of the App object
      App a = new App();

      // This malicious code enters a lock on 
      // the object but never exits the lock
      Monitor.Enter(a);

      // For demonstration purposes, let's release the 
      // root to this object and force a garbage collection
      a = null;
      GC.Collect();

      // For demonstration purposes, wait until all Finalize
      // methods have completed their execution - deadlock!
      GC.WaitForPendingFinalizers();

      // We never get to the line of code below!
      Console.WriteLine("Leaving Main");
   }

   // This is the App type's Finalize method
   ~App() {
      // For demonstration purposes, have the CLR's 
      // Finalizer thread attempt to lock the object.
      // NOTE: Since the Main thread owns the lock, 
      // the Finalizer thread is deadlocked!
      lock (this) {
         // Pretend to do something in here...
      }
   }
}

Answer 4

Here's one other interesting thing related to this topic:

Questionable value of SyncRoot on Collections (by Brad Adams):

You’ll notice a SyncRoot property on many of the Collections in System.Collections. In retrospeced, I think this property was a mistake. Krzysztof Cwalina, a Program Manger on my team, just sent me some thoughts on why that is – I agree with him:

We found the SyncRoot-based synchronization APIs to be insufficiently flexible for most scenarios. The APIs allow for thread safe access to a single member of a collection. The problem is that there are numerous scenarios where you need to lock on multiple operations (for example remove one item and add another). In other words, it’s usually the code that uses a collection that wants to choose (and can actually implement) the right synchronization policy, not the collection itself. We found that SyncRoot is actually used very rarely and in cases where it is used, it actually does not add much value. In cases where it’s not used, it is just an annoyance to implementers of ICollection.

Rest assured we will not make the same mistake as we build the generic versions of these collections

Answer 5

Another concrete example:

class Program
{
    public class Test
    {
        public string DoThis()
        {
            lock (this)
            {
                return "got it!";
            }
        }
    }

    public delegate string Something();

    static void Main(string[] args)
    {
        var test = new Test();
        Something call = test.DoThis;
        //Holding lock from _outside_ the class
        IAsyncResult async;
        lock (test)
        {
            //Calling method on another thread.
            async = call.BeginInvoke(null, null);
        }
        async.AsyncWaitHandle.WaitOne();
        string result = call.EndInvoke(async);

        lock (test)
        {
            async = call.BeginInvoke(null, null);
            async.AsyncWaitHandle.WaitOne();
        }
        result = call.EndInvoke(async);
    }
}

In this example, the first call will succeed, but if you trace in the debugger the call to DoSomething will block until the lock is release. The second call will deadlock, since the Main thread is holding the monitor lock on test.

The issue is that Main can lock the object instance, which means that it can keep the instance from doing anything that the object thinks should be synchronized. The point being that the object itself knows what requires locking, and outside interference is just asking for trouble. That's why the pattern of having a private member variable that you can use exclusively for synchronization without having to worry about outside interference.

The same goes for the equivalent static pattern:

class Program
{
    public static class Test
    {
        public static string DoThis()
        {
            lock (typeof(Test))
            {
                return "got it!";
            }
        }
    }

    public delegate string Something();

    static void Main(string[] args)
    {
        Something call =Test.DoThis;
        //Holding lock from _outside_ the class
        IAsyncResult async;
        lock (typeof(Test))
        {
            //Calling method on another thread.
            async = call.BeginInvoke(null, null);
        }
        async.AsyncWaitHandle.WaitOne();
        string result = call.EndInvoke(async);

        lock (typeof(Test))
        {
            async = call.BeginInvoke(null, null);
            async.AsyncWaitHandle.WaitOne();
        }
        result = call.EndInvoke(async);
    }
}

Use a private static object to synchronize on, not the Type.