lock (this) – For and against

Recently reviewing the code I have found the following construction:

lock (this)
{
  // Do something
}

To synchronize access to shared resource object is used, in which this construction occur. Theoretically, this code is correct. What more, there was no problem with deadlock in the program.

Reviewing the literature we find the following examples:

1. In O’Reilly Programming C# we can found following example of using the keyword lock:

public void Incrementer()
{
  try
  {
    while (counter < 1000)
    {
      int temp;
      lock (this)
        {
          temp = counter;
          temp++;
          Thread.Sleep(1);
          counter = temp;
        }

        Console.WriteLine(“{0}. Incrementer: {1}”, Thread.CurrentThread.Name, temp);
    }
  }
}&#91;/code&#93;
<p style="text-align: justify;">In addition to the above example, the book does not discuss its advantages and disadvantages.</p>
<p style="text-align: justify;">2. Andrew Troelsen in Pro C# 2008 and the .NET 3.5 Platform recommends using <em>lock (this)</em> in the case of private methods:</p>
[code lang="csharp"]private void SomePrivateMethod()
{
  lock (this)
  {
    // do something
  }
}

In both cases, the threat is ignored, which entails the use of lock (this). Let’s analyze the code below:

public class BadLock
{
  public void Compute()
  {
    Console.WriteLine("Compute - Start");
    lock (this)
    {
      Console.WriteLine("Compute - In Lock");
      Thread.Sleep(5000);
    }

    Console.WriteLine("Compute - Stop");
  }
}

Class BadLock has in the method Compute, which simulates the process of calculation and the uses to synchronize lock (this). In most cases, the use of this code should not cause problems. However, there is a scenario, which can lead to deadlock. In a situation, when we create an object of type BadLock and we use it to ensure synchronization, and then execute a method of class BadLock can lead to deadlock. An example of such a scenario presented in the following code:

public static void BadLockTest()
{
  Console.WriteLine("Bad Lock test 1");
  BadLock badLock = new BadLock();
  new Thread(new ParameterizedThreadStart(BackgroundMethod)).Start(badLock);
  Thread.Sleep(500);
  badLock.Compute();
}

private static void BackgroundMethod(Object lockable)
{
  lock (lockable)
  {
    Console.Out.WriteLine("BackgroundMethod - Lock");
    Thread.Sleep(Timeout.Infinite);
  }
}

When you start an application we obtain the following result:

Bad Lock test 1
BackgroundMethod - Lock
Compute – Start

No matter how long we will wait, additional messages will not be displayed.

The solution to this problem is to replace the structure:

lock (this)
{
  ...
}

with following design:

private object synchronizationObject = new Object();
...
lock (synchronizationObject)
{
  ...
}

Only after you apply this construction we are sure, that no one will break the state of the object and will not lead to deadlock. After you change the object BadLock will look like this:

public class GoodLock
{
  private readonly Object synchronizationObject = new Object();

  public void Compute()
  {
    Console.WriteLine("Compute - Start");
    lock (synchronizationObject)
    {
      Console.WriteLine("Compute - In Lock");
      Thread.Sleep(5000);
    }

    Console.WriteLine("Compute - Stop");
  }
}

and get the correct results:

Good Lock test 1
BackgroundMethod - Lock
Compute - Start
Compute - In Lock
Compute – Stop

At the end of the topic, still one thing remains associated with the method BackgroundMethod. I am not in favor of defensive programming, but in this case, instead of setting lock on the object, I would suggest we use also create an independent object used to synchronize.

Check it:

Parallel.For – Multithreading has never been so easy Enum and default initialization value Ping Application on two monitors