What's the use of the SyncRoot pattern?

Question

I am mocking a response using wireMock and using a json file that is mapped. I need the output of the dates to look like this "date": "2022-06-29T05:00:00Z",

In my json file, I have this and am getting a 500 because the formatter is wrong. my file: "date": "{{now format='yyyy-MM-dd'T'HH:mm:ssZ'}}",

I also need this to work for a date that I add 1 day to: "date": "{{now offset='1 days' format='yyyy-MM-dd'T'HH:mm:ssZ'}}",

How should I fix the date field in the json to get the right output?

Solution 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.

Solution 2

The actual purpose of this pattern is implementing correct synchronization with wrappers hierarchy.

For example, if class WrapperA wraps an instance of ClassThanNeedsToBeSynced, and class WrapperB wraps the same instance of ClassThanNeedsToBeSynced, you can't lock on WrapperA or WrapperB, since if you lock on WrapperA, lock on WrappedB won't wait. For this reason you must lock on wrapperAInst.SyncRoot and wrapperBInst.SyncRoot, which delegate lock to ClassThanNeedsToBeSynced's one.

Example:

public interface ISynchronized
{
    object SyncRoot { get; }
}

public class SynchronizationCriticalClass : ISynchronized
{
    public object SyncRoot
    {
        // you can return this, because this class wraps nothing.
        get { return this; }
    }
}

public class WrapperA : ISynchronized
{
    ISynchronized subClass;

    public WrapperA(ISynchronized subClass)
    {
        this.subClass = subClass;
    }

    public object SyncRoot
    {
        // you should return SyncRoot of underlying class.
        get { return subClass.SyncRoot; }
    }
}

public class WrapperB : ISynchronized
{
    ISynchronized subClass;

    public WrapperB(ISynchronized subClass)
    {
        this.subClass = subClass;
    }

    public object SyncRoot
    {
        // you should return SyncRoot of underlying class.
        get { return subClass.SyncRoot; }
    }
}

// Run
class MainClass
{
    delegate void DoSomethingAsyncDelegate(ISynchronized obj);

    public static void Main(string[] args)
    {
        SynchronizationCriticalClass rootClass = new SynchronizationCriticalClass();
        WrapperA wrapperA = new WrapperA(rootClass);
        WrapperB wrapperB = new WrapperB(rootClass);

        // Do some async work with them to test synchronization.

        //Works good.
        DoSomethingAsyncDelegate work = new DoSomethingAsyncDelegate(DoSomethingAsyncCorrectly);
        work.BeginInvoke(wrapperA, null, null);
        work.BeginInvoke(wrapperB, null, null);

        // Works wrong.
        work = new DoSomethingAsyncDelegate(DoSomethingAsyncIncorrectly);
        work.BeginInvoke(wrapperA, null, null);
        work.BeginInvoke(wrapperB, null, null);
    }

    static void DoSomethingAsyncCorrectly(ISynchronized obj)
    {
        lock (obj.SyncRoot)
        {
            // Do something with obj
        }
    }

    // This works wrong! obj is locked but not the underlaying object!
    static void DoSomethingAsyncIncorrectly(ISynchronized obj)
    {
        lock (obj)
        {
            // Do something with obj
        }
    }
}

Solution 3

Here is an example :

class ILockMySelf
{
    public void doThat()
    {
        lock (this)
        {
            // Don't actually need anything here.
            // In this example this will never be reached.
        }
    }
}

class WeveGotAProblem
{
    ILockMySelf anObjectIShouldntUseToLock = new ILockMySelf();

    public void doThis()
    {
        lock (anObjectIShouldntUseToLock)
        {
            // doThat will wait for the lock to be released to finish the thread
            var thread = new Thread(x => anObjectIShouldntUseToLock.doThat());
            thread.Start();

            // doThis will wait for the thread to finish to release the lock
            thread.Join();
        }
    }
}

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.

Solution 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 (sic), 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.

Solution 5

See this Jeff Richter's article. More specifically, this example which demonstrates that locking on "this" can cause a deadlock:

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...
      }
   }
}

View more solutions

Comments

[haze4real]

The SyncRoot on Collections isn't the same this topic is talking about, making the private field SyncRoot public is as bad as locking on "this". The topic is comparing locking on private readonly field to locking on "this", in case of Collections the private field was made accessable through a public property, which isn't best practice and can lead to deadlocks.

[prabhakaran]

@Haze Can you explain the part "was made accessable through a public property, which isn't best practice and can lead to deadlocks". How a deadlock can happen here?

[haze4real]

@prabhakaran The problem with locking on this, a public field or a private field exposed through a public property is that anyone can access it, which might lead to deadlocks if you aren't aware of the implementation. Classes shouldn't be designed in a way that the knowledge of the implementation of the class is required for its correct use.

[haze4real]

@prabhakaran For a concrete example look at Darren Clark's answer.

[mistertodd]

OK. Where does SyncRoot come in? Why did the .NET FCL designers create .SyncRoot? Or, to paraphrase Ryan's question, "What's the use of the SyncRoot pattern?"

[Govert]

Thank you for actually answering the question!

[Bogdan Mart]

if that's bad, than why CLR designers created lock keywoard in first place? Why didn't they created class Lock, instance of which you can place into _syncRoot field? And that would remove locking structures overhead from each Object's header

[Jirka Hanika]

@IanBoyd - The .SyncRoot gives developers an explicit choice, whenever they are locking the collection (either itself or its SyncRoot) to state whether they want to interact with any locking inside the code that implements the collection, or be guaranteed not to interact with that. This primarily affects code implementing collections that wrap other collections (inheritance, decorators, composites...) Not saying whether this is good or bad. Saying that most users of collections do not need to care.

[Jirka Hanika]

Link-only answer. Example showing that malicious code can be constructed so as to deadlock. And the link is dangling.