lock (this) - For and against

13Jan/1211

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 occure.. 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);
    }
  }
}

In addition to the above example, the book does not discuss its advantages and disadvantages.

2. Andrew Troelsen in Pro C# 2008 and the .NET 3.5 Platform recommends using lock (this) in the case of private methods:

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.

Tagged as: C#, deadlock, lock, synchronization, multithreaded Leave a comment

Comments (11) Trackbacks (0) ( subscribe to comments on this post )

Paul
January 13th, 2012 - 14:47

Post Title should read – lock(this) – against. There is no a.

Paul

( REPLY )
wojtek(szogun1987)
January 13th, 2012 - 15:44

For a deadlock occurs only when the method BackgroundMethod never ends. So basically, and such a code even if he is correct then it should not be closed in locku. The reason for the lock structure(this) is invalid is the fact that in the example above callstack indicates our method and not the method that really is the cause of the error. And we as creators of the method we have for some time to deal with it and not the person who wrote the method BackgroundMethod.

( REPLY )
- janosz
  January 14th, 2012 - 19:30
  
  The example is very simplified. I could not find another easy solution to this illustrative example.
  
  While rightly noticed, that causes a big problem, if you come to a deadlock somewhere and will need to find fault. In addition, the difficulty will be even greater if we use the foreign dll.
  
  ( REPLY )
Andrew
January 14th, 2012 - 18:51

Aside from all the disadvantages of lock(this) has one advantage (so gentlemen purists, advantage!): is quick to write. And even there is justification for this approach: prototypes, PoC and small applications.

What is more: speaking, You can not do that lock(this) I can do another object lock on that instance it is meaningless, bo to samo w sobie jest złamaniem innej zasady “assume no lock on objects outside your control”. (incidentally, this is a rule in my opinion precludes lock(this), becthis outside Singleton does not create any instance of itself). “assume no lock on objects outside your control”. (incidentally, this is a rule in my opinion precludes lock(this), because outside Singleton does not create any instance of itself).

I in 99% cases apply a special lock on the object for this purpose – ever, there is no problem.

( REPLY )
- janosz
  January 14th, 2012 - 19:39
  
  Somehow I have a bad experience with makeshift solutions. Generally, they are able to live very long. Problems with debt that technology can typically occur in the least appropriate moment.
  
  According to me one legitimate solution is to use a special lock on the object.
  
  I am worried about the fact, that 2 Books, which are often recommended to learn programming in C # containing structures, that should not be used.
  
  ( REPLY )
  - Andrew
    January 14th, 2012 - 20:59
    
    The idea of a prototype or PoC code assumes thrown into the trash. If someone I break this rule is (generalizes) lock design(this) may be the smallest problem.
    
    “one legitimate solution is ”
    - I do not like this type of wording. Probably, after the recess in the discussion, a few examples, itd, after a long time would be defended, from the beginning that you wanted “only” for some specific conditions such >business applications, from medium to long term, written by more than 1 person, itdz share'owaniem lock'a no longer qualifies for…<
    
    However,, I during the code review was something like the 101% I would tell it to change, at least when it, not dazzling to the eyes and did not give bad example to other (until it is surprising what they can dev copy of someone else's code .
    
    Summary: what I mean… construction is bad, but should also write in what conditions. It's as if simply write "goto is evil".
    
    ( REPLY )
    - janosz
      January 14th, 2012 - 21:17
      
      With this "one legitimate solution is" you're right.
      
      The answer should be a programmer “it depends” .
      
      ( REPLY )
Darek Tarczyński
January 17th, 2012 - 11:10

I, that the problem with the use of lock(this) lies in: firstly – his nierozumieniu, second, ease of use. If you're new in fact that construction, and that this construction podejrzały somewhere in the dark hoping to use it, that it performs a MAGIC – which does not.
With regard to what he wrote Adam. And why use this design in the prototype? The use of the synchronization object is really 1 line more.

( REPLY )
- Andrew
  January 17th, 2012 - 19:38
  
  Maybe one line, but you scroolować screen at the beginning of class
  
  But it's really not the point, if you do locks(this), and someone else to do lock(instance) as in the example, are one and the other person breaks a rule. From what it really breaks the second rule “more”. Articles should be “Do not lock() for instance, beyond your control” (because that is the principle).
  
  btw: I see no reason to use this design prototypes (otherwise, that is incorrect in the code production)? The prototype is to throw, be written as soon as possible. If someone writes without hesitation lock(this) and it works, This praise him for it.
  
  ( REPLY )
Darek Tarczyński
January 18th, 2012 - 16:02

@ Andrew
The prototype prototype, but why use the stupid practice? Someone on the basis of a prototype (eg.. less experienced programmer) will develop the production version – and copies, he works – and do not blame him at all. If it gets a scolding, it wrong, I wrote something on the basis of the prototype should not again spend hours analyzing each line and meditation, if this line is not a time bomb – there is no time for that when used in prototyping (not believe, that throwing a prototype code and begin anew).

( REPLY )
Andrew
January 20th, 2012 - 19:04

Okay, and how the prototype will not be handling errors and a programmer to copy this wine will also be the person creating the prototype?

Secondly: has no right to be broke “lock(this)”, is only a loose guideline “not use an instance beyond own control to the objects synchonizujących”. The construction is błędo-Genna, therefore it should not be used in production code, and so.

The argument of the copy of the prototype is misses, because copying the code from the prototype is much larger error, than this loose guideline for locks,.

And so, prototype is thrown away, otherwise it is not a prototype. Although seldom use prototypes as such.

( REPLY )