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

The MSDN documentation says that

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

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?

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 http://msdn.microsoft.com/en-us/library/ms173179.aspx">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();

Locking on the this pointer can be bad if you are locking over a shared resource. A shared resource can be a static variable or a file on your computer - i.e. something that is shared between all users of the class. The reason is that the this pointer will contain a different reference to a location in memory each time your class is instantiated. So, locking over this in once instance of a class is different than locking over this in another instance of a class.

Check out this code to see what I mean. Add the following code to your main program in a Console application:

    static void Main(string[] args)
    {
         TestThreading();
         Console.ReadLine();
    }

    public static void TestThreading()
    {
        Random rand = new Random();
        Thread[] threads = new Thread[10];
        TestLock.balance = 100000;
        for (int i = 0; i < 10; i++)
        {
            TestLock tl = new TestLock();
            Thread t = new Thread(new ThreadStart(tl.WithdrawAmount));
            threads[i] = t;
        }
        for (int i = 0; i < 10; i++)
        {
            threads[i].Start();
        }
        Console.Read();
    }

Create a new class like the below.

 class TestLock
{
    public static int balance { get; set; }
    public static readonly Object myLock = new Object();

    public void Withdraw(int amount)
    {
      // Try both locks to see what I mean
      //             lock (this)
       lock (myLock)
        {
            Random rand = new Random();
            if (balance >= amount)
            {
                Console.WriteLine("Balance before Withdrawal :  " + balance);
                Console.WriteLine("Withdraw        : -" + amount);
                balance = balance - amount;
                Console.WriteLine("Balance after Withdrawal  :  " + balance);
            }
            else
            {
                Console.WriteLine("Can't process your transaction, current balance is :  " + balance + " and you tried to withdraw " + amount);
            }
        }

    }
    public void WithdrawAmount()
    {
        Random rand = new Random();
        Withdraw(rand.Next(1, 100) * 100);
    }
}

Here is a run of the program locking on this.

   Balance before Withdrawal :  100000
    Withdraw        : -5600
    Balance after Withdrawal  :  94400
    Balance before Withdrawal :  100000
    Balance before Withdrawal :  100000
    Withdraw        : -5600
    Balance after Withdrawal  :  88800
    Withdraw        : -5600
    Balance after Withdrawal  :  83200
    Balance before Withdrawal :  83200
    Withdraw        : -9100
    Balance after Withdrawal  :  74100
    Balance before Withdrawal :  74100
    Withdraw        : -9100
    Balance before Withdrawal :  74100
    Withdraw        : -9100
    Balance after Withdrawal  :  55900
    Balance after Withdrawal  :  65000
    Balance before Withdrawal :  55900
    Withdraw        : -9100
    Balance after Withdrawal  :  46800
    Balance before Withdrawal :  46800
    Withdraw        : -2800
    Balance after Withdrawal  :  44000
    Balance before Withdrawal :  44000
    Withdraw        : -2800
    Balance after Withdrawal  :  41200
    Balance before Withdrawal :  44000
    Withdraw        : -2800
    Balance after Withdrawal  :  38400

Here is a run of the program locking on myLock.

Balance before Withdrawal :  100000
Withdraw        : -6600
Balance after Withdrawal  :  93400
Balance before Withdrawal :  93400
Withdraw        : -6600
Balance after Withdrawal  :  86800
Balance before Withdrawal :  86800
Withdraw        : -200
Balance after Withdrawal  :  86600
Balance before Withdrawal :  86600
Withdraw        : -8500
Balance after Withdrawal  :  78100
Balance before Withdrawal :  78100
Withdraw        : -8500
Balance after Withdrawal  :  69600
Balance before Withdrawal :  69600
Withdraw        : -8500
Balance after Withdrawal  :  61100
Balance before Withdrawal :  61100
Withdraw        : -2200
Balance after Withdrawal  :  58900
Balance before Withdrawal :  58900
Withdraw        : -2200
Balance after Withdrawal  :  56700
Balance before Withdrawal :  56700
Withdraw        : -2200
Balance after Withdrawal  :  54500
Balance before Withdrawal :  54500
Withdraw        : -500
Balance after Withdrawal  :  54000

There is very good article about it http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects by Rico Mariani, performance architect for the Microsoft® .NET runtime

Excerpt:

> The basic problem here is that you don't own the type object, and you > don't know who else could access it. In general, it's a very bad idea > to rely on locking an object you didn't create and don't know who else > might be accessing. Doing so invites deadlock. The safest way is to > only lock private objects.

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

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.

Sorry guys but I can't agree with the argument that locking this might cause deadlock. You are confusing two things: deadlocking and starving.

• You cannot cancel deadlock without interrupting one of the threads so after you get into a deadlock you cannot get out
• Starving will end automatically after one of the threads finishes its job

Here is a picture which illustrates the difference.

Conclusion
You can still safely use lock(this) if thread starvation is not an issue for you. You still have to keep in mind that when the thread, which is starving thread using lock(this) ends in a lock having your object locked, it will finally end in eternal starvation ;)

Here is some sample code that is simpler to follow (IMO): (Will work in LinqPad, reference following namespaces: System.Net and System.Threading.Tasks)

Something to remember is that lock(x) basically is syntactic sugar and what it does is to use Monitor.Enter and then uses a try, catch, finally block to call Monitor.Exit. See: https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor.enter (remarks section)

> or use the C# lock statement (SyncLock statement in Visual Basic), > which wraps the Enter and Exit methods in a try…finally block.

void Main()
{
	//demonstrates why locking on THIS is BADD! (you should never lock on something that is publicly accessible)
	ClassTest test = new ClassTest();
	lock(test) //locking on the instance of ClassTest
	{
		Console.WriteLine($"CurrentThread {Thread.CurrentThread.ManagedThreadId}");
		Parallel.Invoke(new Action[]
		{
			() => {
				//this is there to just use up the current main thread. 
				Console.WriteLine($"CurrentThread {Thread.CurrentThread.ManagedThreadId}");
				},
			//none of these will enter the lock section.
			() => test.DoWorkUsingThisLock(1),//this will dead lock as lock(x) uses Monitor.Enter
			() => test.DoWorkUsingMonitor(2), //this will not dead lock as it uses Montory.TryEnter
		});
	}
}

public class ClassTest
{
	public void DoWorkUsingThisLock(int i)
	{
		Console.WriteLine($"Start ClassTest.DoWorkUsingThisLock {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
		lock(this) //this can be bad if someone has locked on this already, as it will cause it to be deadlocked!
		{
			Console.WriteLine($"Running: ClassTest.DoWorkUsingThisLock {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
			Thread.Sleep(1000);
		}
		Console.WriteLine($"End ClassTest.DoWorkUsingThisLock Done {i}  CurrentThread {Thread.CurrentThread.ManagedThreadId}");
	}

	public void DoWorkUsingMonitor(int i)
	{
		Console.WriteLine($"Start ClassTest.DoWorkUsingMonitor {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
		if (Monitor.TryEnter(this))
		{
			Console.WriteLine($"Running: ClassTest.DoWorkUsingMonitor {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
			Thread.Sleep(1000);
			Monitor.Exit(this);
		}
		else
		{
			Console.WriteLine($"Skipped lock section!  {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
		}

		Console.WriteLine($"End ClassTest.DoWorkUsingMonitor Done {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
		Console.WriteLine();
	}
}

Output

CurrentThread 15
CurrentThread 15
Start ClassTest.DoWorkUsingMonitor 2 CurrentThread 13
Start ClassTest.DoWorkUsingThisLock 1 CurrentThread 12
Skipped lock section!  2 CurrentThread 13
End ClassTest.DoWorkUsingMonitor Done 2 CurrentThread 13

Notice that Thread#12 never ends as its dead locked.

Please refer to the following link which explains why lock (this) is not a good idea.

https://docs.microsoft.com/en-us/dotnet/standard/threading/managed-threading-best-practices

So the solution is to add a private object, for example, lockObject to the class and place the code region inside the lock statement as shown below:

lock (lockObject)
{
...
}

Here's a much simpler illustration (taken from Question 34 here) why lock(this) is bad and may result in deadlocks when consumer of your class also try to lock on the object. Below, only one of three thread can proceed, the other two are deadlocked.

> class SomeClass > { > public void SomeMethod(int id) > { > lock(this) > { > while(true) > { > Console.WriteLine("SomeClass.SomeMethod #" + id); > } > } > } > } > > class Program > { > static void Main(string[] args) > { > SomeClass o = new SomeClass(); >
> lock(o) > { > for (int threadId = 0; threadId < 3; threadId++) > { > Thread t = new Thread(() => { > o.SomeMethod(threadId); > }); > t.Start(); > } >
> Console.WriteLine(); > }

To work around, this guy used Thread.TryMonitor (with timeout) instead of lock:

> Monitor.TryEnter(temp, millisecondsTimeout, ref lockWasTaken); > if (lockWasTaken) > { > doAction(); > } > else > { > throw new Exception("Could not get lock"); > }

https://blogs.appbeat.io/post/c-how-to-lock-without-deadlocks

You can establish a rule that says that a class can have code that locks on 'this' or any object that the code in the class instantiates. So it's only a problem if the pattern is not followed.

If you want to protect yourself from code that won't follow this pattern, then the accepted answer is correct. But if the pattern is followed, it's not a problem.

The advantage of lock(this) is efficiency. What if you have a simple "value object" that holds a single value. It's just a wrapper, and it gets instantiated millions of times. By requiring the creation of a private sync object just for locking, you've basically doubled the size of the object and doubled the number of allocations. When performance matters, this is an advantage.

When you don't care about number of allocations or memory footprint, avoiding lock(this) is preferable for the reasons indicated in other answers.

Here is why it is not recommended.
To explain it more according to the following code snippet, consider you wrote a class (SomeClass in this example) and consumer of your class (a coder named "John") wants to acquire a lock over an instance of your class (someObject in this example). He encounters a deadlock because he gets a lock over instance someObject and inside this lock he calls a method of that instance (SomeMethod()) which internally acquires a lock over the exact same instance.

• I could have written the following example with or without Task/Thread and the gist of deadlock still remains the same.

• To prevent bizarre situation where the main Thread finishes while its children are still running, I used .Wait(). However, in long-running-tasks or situation where a code-snippet executes more frequently, you would definitely see the same behavior.

• Although John applied a bad practice of using an instance of a class as a lock-object, but we (as the developer of a classlibrary SomeClass) should deter such situation simple by not using this as a lock-object in our class.

• Instead, we should declare a simple private field and use that as our lock-object.

   using System;
   using System.Threading;
   using System.Threading.Tasks;
  
   class SomeClass
   {
       public void SomeMethod()
       {
           //NOTE: Locks over an object that is already locked by the caller.
           //      Hence, the following code-block never executes.
           lock (this)
           {
               Console.WriteLine("Hi");
           }
       }
   }
  
   public class Program
   {
       public static void Main()
       {
           SomeClass o = new SomeClass();
  
           lock (o)
           {
               Task.Run(() => o.SomeMethod()).Wait();
           }
  
           Console.WriteLine("Finish");
       }
   }
  

There will be a problem if the instance can be accessed publicly because there could be other requests that might be using the same object instance. It's better to use private/static variable.