Why is lock(this) {...} bad?

Question

The MSDN documentation says that

public class SomeObject
{
  public void SomeOperation()
  {
    lock(this)
    {
      //Access instance variables
    }
  }
}

is "is a problem if the instance can be accessed publicly". I'm wondering why? Is it because the lock will be held longer than necessary? Or is there some more insidious reason?

Answer 1

Because if people can get at your object instance (ie: your this) pointer, then they can also try to lock that same object. Now they might not be aware that you're locking on this internally, so this may cause problems (possibly a deadlock)

In addition to this, it's also bad practice, because it's locking "too much"

For example, you might have a member variable of List<int>, and the only thing you actually need to lock is that member variable. If you lock the entire object in your functions, then other things which call those functions will be blocked waiting for the lock. If those functions don't need to access the member list, you'll be causing other code to wait and slow down your application for no reason at all.

Answer 2

...and the exact same arguments apply to this construct as well:

lock(typeof(SomeObject))

Answer 3

Take a look at the MSDN Topic Thread Synchronization (C# Programming Guide)

Generally, it is best to avoid locking on a public type, or on object instances beyond the control of your application. For example, lock(this) can be problematic if the instance can be accessed publicly, because code beyond your control may lock on the object as well. This could create deadlock situations where two or more threads wait for the release of the same object. Locking on a public data type, as opposed to an object, can cause problems for the same reason. Locking on literal strings is especially risky because literal strings are interned by the common language runtime (CLR). This means that there is one instance of any given string literal for the entire program, the exact same object represents the literal in all running application domains, on all threads. As a result, a lock placed on a string with the same contents anywhere in the application process locks all instances of that string in the application. As a result, it is best to lock a private or protected member that is not interned. Some classes provide members specifically for locking. The Array type, for example, provides SyncRoot. Many collection types provide a SyncRoot member as well.

Answer 4

Because any chunk of code that can see the instance of your class can also lock on that reference. You want to hide (encapsulate) your locking object so that only code that needs to reference it can reference it. The keyword this refers to the current class instance, so any number of things could have reference to it and could use it to do thread synchronization.

To be clear, this is bad because some other chunk of code could use the class instance to lock, and might prevent your code from obtaining a timely lock or could create other thread sync problems. Best case: nothing else uses a reference to your class to lock. Middle case: something uses a reference to your class to do locks and it causes performance problems. Worst case: something uses a reference of your class to do locks and it causes really bad, really subtle, really hard-to-debug problems.

Answer 5

There's also some good discussion about this here: Is this the proper use of a mutex?

Answer 6

It is bad form to use this in lock statements because it is generally out of your control who else might be locking on that object.

In order to properly plan parallel operations, special care should be taken to consider possible deadlock situations, and having an unknown number of lock entry points hinders this. For example, any one with a reference to the object can lock on it without the object designer/creator knowing about it. This increases the complexity of multi-threaded solutions and might affect their correctness.

A private field is usually a better option as the complier will enforce access restrictions to it, and it will encapsulate the locking mechanism. Using this violates encapsulation by exposing part of your locking implementation to the public. It is also not clear that you will be acquiring a lock on this unless it has been documented. Even then, relying on documentation to prevent a problem is sub-optimal.

Finally, there is the common misconception that lock(this) actually modifies the object passed as a parameter, and in some way makes it read-only or inaccessible. This is false. The object passed as a parameter to lock merely serves a key. If a lock is already being held on that key, the lock cannot be made, otherwise, the lock is allowed.

Run the following C# code as an example.

public class Person
{
 private int age;
 private string name;

 public int Age
 {
  get { return age; }
  set { age = value; }
 }
 public string Name
 {
  get { return name; }
  set { name = value; }
 }

 public Person(string name)
 {
  this.age = 0;
  this.name = name;
 }

 public void LockThis()
 {
  lock (this)
  {
   System.Threading.Thread.Sleep(10000);
  }
 }

 public void GrowOld()
 {
  this.age++;
 }
}

class Program
{
 static void Main(string[] args)
 {
  Person nancy = new Person("Nancy Drew");
  nancy.Age = 15;
  Thread a = new Thread(nancy.LockThis);
  Thread b = new Thread(Program.Timewarp);
  Thread c = new Thread(Program.NameChange);
  a.Start();
  b.Start(nancy);
  c.Start(nancy);
  a.Join();
  Console.ReadLine();
 }

 static void Timewarp(object subject)
 {
  Person person = subject as Person;
  if (person == null) throw new ArgumentNullException("subject");
  lock (person.Name)
  {
   while (person.Age <= 23)
   {
    if (Monitor.TryEnter(person, 10) == false)
    {
     Console.WriteLine("'this' person is locked!");
    }
    else Monitor.Exit(person);
    person.GrowOld();
    Console.WriteLine("{0} is {1} years old.", person.Name, person.Age);
   }
  }
 }

 static void NameChange(object subject)
 {
  Person person = subject as Person;
  if (person == null) throw new ArgumentNullException("subject");
  // Be careful about using strings as locks
  if (Monitor.TryEnter(person.Name, 30) == false)
  {
   Console.WriteLine("Failed to obtain lock on 'person.Name' on first try, wait longer.");
  }
  else Monitor.Exit(person.Name);
  if (Monitor.TryEnter("Nancy Drew", 30) == false)
  {
   Console.WriteLine("Failed to obtain lock using 'Nancy Drew' literal, locked by 'person.Name' since both are the same thanks to inlining!");
  }
  else Monitor.Exit("Nancy Drew");

  if (Monitor.TryEnter(person.Name, 10000))
  {
   string oldName = person.Name;
   person.Name = "Nancy Callahan";
   Console.WriteLine("Name changed from '{0}' to '{1}'.", oldName, person.Name);
  }
  else Monitor.Exit(person.Name);
 }
}