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 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.
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.
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.
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'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 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)
{
...
}
