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

Question

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 compiler 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 as a key. If a lock is already being held on that key, the lock cannot be made; otherwise, the lock is allowed.

This is why it's bad to use strings as the keys in lock statements, since they are immutable and are shared/accessible across parts of the application. You should use a private variable instead, an Object instance will do nicely.

Run the following C# code as an example.

public class Person
{
    public int Age { get; set;  }
    public string Name { get; set; }

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

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

    static void Timewarp(object subject)
    {
        var person = subject as Person;
        if (person == null) throw new ArgumentNullException("subject");
        // A lock does not make the object read-only.
        lock (person.Name)
        {
            while (person.Age <= 23)
            {
                // There will be a lock on 'person' due to the LockThis method running in another thread
                if (Monitor.TryEnter(person, 10) == false)
                {
                    Console.WriteLine("'this' person is locked!");
                }
                else Monitor.Exit(person);
                person.Age++;
                if(person.Age == 18)
                {
                    // Changing the 'person.Name' value doesn't change the lock...
                    person.Name = "Nancy Smith";
                }
                Console.WriteLine("{0} is {1} years old.", person.Name, person.Age);
            }
        }
    }

    static void NameChange(object subject)
    {
        var person = subject as Person;
        if (person == null) throw new ArgumentNullException("subject");
        // You should avoid locking on strings, since they are immutable.
        if (Monitor.TryEnter(person.Name, 30) == false)
        {
            Console.WriteLine("Failed to obtain lock on 50 year old Nancy, because Timewarp(object) locked on string \"Nancy Drew\".");
        }
        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 object 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);
    }
}

Console output

'this' person is locked!
Nancy Drew is 16 years old.
'this' person is locked!
Nancy Drew is 17 years old.
Failed to obtain lock on 50 year old Nancy, because Timewarp(object) locked on string "Nancy Drew".
'this' person is locked!
Nancy Smith is 18 years old.
'this' person is locked!
Nancy Smith is 19 years old.
'this' person is locked!
Nancy Smith is 20 years old.
Failed to obtain lock using 'Nancy Drew' literal, locked by 'person.Name' since both are the same object thanks to inlining!
'this' person is locked!
Nancy Smith is 21 years old.
'this' person is locked!
Nancy Smith is 22 years old.
'this' person is locked!
Nancy Smith is 23 years old.
'this' person is locked!
Nancy Smith is 24 years old.
Name changed from 'Nancy Drew' to 'Nancy Callahan'.

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.

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.

I know this is an old thread, but because people can still look this up and rely on it, it seems important to point out that lock(typeof(SomeObject)) is significantly worse than lock(this). Having said that; sincere kudos to Alan for pointing out that lock(typeof(SomeObject)) is bad practice.

An instance of System.Type is one of the most generic, coarse-grained objects there is. At the very least, an instance of System.Type is global to an AppDomain, and .NET can run multiple programs in an AppDomain. This means that two entirely different applications could potentially cause interference in one another even to the extent of creating a deadlock if they both try to get a synchronization lock on the same global instance of System.Type.

So lock(this) isn't particularly robust form, can cause problems and should always raise eyebrows for all the reasons cited. Yet there is widely used, relatively well-respected and apparently stable code like log4net that uses the lock(this) pattern extensively, even though I would personally prefer to see that pattern change.

But lock(typeof(SomeObject)) opens up a whole new and enhanced can of worms.

For what it's worth.

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

lock(typeof(SomeObject))

Imagine that you have a skilled secretary at your office that's a shared resource in the department. Once in a while, you rush towards them because you have a task, only to hope that another one of your co-workers has not already claimed them. Usually you only have to wait for a brief period of time.

Because caring is sharing, your manager decides that customers can use the secretary directly as well. But this has a side effect: A customer might even claim them while you're working for this customer and you also need them to execute part of the tasks. A deadlock occurs, because claiming is no longer a hierarchy. This could have been avoided all together by not allowing customers to claim them in the first place.

lock(this) is bad as we've seen. An outside object might lock on the object and since you don't control who's using the class, anyone can lock on it... Which is the exact example as described above. Again, the solution is to limit exposure of the object. However, if you have a private, protected or internal class you could already control who is locking on your object, because you're sure that you've written your code yourself. So the message here is: don't expose it as public. Also, ensuring that a lock is used in similar scenario's avoids deadlocks.

The complete opposite of this is to lock on resources that are shared throughout the app domain -- the worst case scenario. It's like putting your secretary outside and allowing everyone out there to claim them. The result is utter chaos - or in terms of source code: it was a bad idea; throw it away and start over. So how do we do that?

Types are shared in the app domain as most people here point out. But there are even better things we can use: strings. The reason is that strings are pooled. In other words: if you have two strings that have the same contents in an app domain, there's a chance that they have the exact same pointer. Since the pointer is used as the lock key, what you basically get is a synonym for "prepare for undefined behavior".

Similarly, you shouldn't lock on WCF objects, HttpContext.Current, Thread.Current, Singletons (in general), etc. The easiest way to avoid all of this? private [static] object myLock = new object();