Why Is Lock(This) {...} Bad

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

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'.

why should we avoid lock(this)?

From http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx:

In general, avoid locking on a public
type, or instances beyond your code's
control. The common constructs lock (this), lock (typeof (MyType)), and
lock ("myLock") violate this
guideline:

lock (this) is a problem if the
instance can be accessed publicly.

lock (typeof (MyType)) is a problem if
MyType is publicly accessible.

lock(“myLock”) is a problem because
any other code in the process using
the same string, will share the same
lock.

Best practice is to define a
private object to lock on, or a
private static object variable to
protect data common to all instances.

Why is it a bad practice to lock the object we are going to change?

From the C# language reference here:

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

lock (this) is a problem if the instance can be accessed publicly.

lock (typeof (MyType)) is a problem if MyType is publicly accessible.

lock("myLock") is a problem because any other code in the process
using the same string, will share the same lock.

Best practice is to define a private object to lock on, or a private
static object variable to protect data common to all instances.

In your case, I would read the above guidance as suggesting that locking on the collection you will be modifying is bad practise. For example, if you wrote this code:

lock (otherProductList) 
{
    otherProductList = new List<IProduct>(); 
}

...then your lock will be worthless. For these reasons it's recommended to use a dedicated object variable for locking.

Note that this doesn't mean your application will break if you use the code you posted. "Best practices" are usually defined to provide easily-repeated patterns that are more technically resilient. That is, if you follow best practice and have a dedicated "lock object," you are highly unlikely to ever write broken lock-based code; if you don't follow best practice then, maybe one time in a hundred, you'll get bitten by an easily-avoided problem.

Additionally (and more generally), code written using best practices is typically more easily modified, because you can be less wary of unexpected side-effects.

Why Locking On a Public Object is a Bad Idea

Well, first off, you could create a third class:

internal class ImplementationDetail
{
    private static readonly object lockme = new object();
    public static void DoDatabaseQuery(whatever)
    {
        lock(lockme)
             ReallyDoQuery(whatever);
    }
}

and now UseSQLKatana and UseSQLCutlass call ImplementationDetail.DoDatabaseQuery.

Second, you could decide to not worry about it, and lock an object that is visible to both types. The primary reason to avoid that is because it becomes difficult to reason about who is locking the object, and difficult to protect against hostile partially trusted code locking the object maliciously. If you don't care about either downside then you don't have to blindly follow the guideline.

Difference between lock(this) and a lock on static object

There could be a big difference. The biggest difference between the two is that the first example uses a single object to lock on (hence the static keyword) while the this keyword in the second example implies locking on an instance. There could therefore be a big difference from a performance perspective and even from a correctness perspective, but that depends on the code inside the lock.

When all you need is to synchronize access to instance level fields, you should not use the static keyword, since that will synchronize the code itself, instead of the data (which could cause an unneeded performance hit). Of course, if the data itself is static (class level data instead of instance level data), you need to use the static keyword. On the other hand, when you use the this keyword to lock, while you're accessing shared / static resources, you will (of course) have a correctness problem, since synchronization is on an instance basis and multiple instance will still be able to access the shared data at the same time.

And there is another problem, but the difference is much smaller than for the previously noted differences. The first example uses a privately declared object to lock on, while the other uses the this pointer, which is the reference to the object of that instance method itself. Because this reference is publicly accessible to other objects, it is possible for them to lock on it, which could cause deadlocks in rare circumstances. If you're an application developer, I wouldn't worry to much about this (as long as you don't lock on things like System.String or System.Type), but if you are a framework developer, you should certainly not use lock(this), since there is no way to tell in what way application developers will (ab)use your code.

c# why put object in the lock statement

I've made a very simple class to illustrate what the object in the lock is there for.

public class Account
{
    private decimal _balance = 0m;
    private object _transactionLock = new object();
    private object _saveLock = new object();

    public void Deposit(decimal amount)
    {
        lock (_transactionLock)
        {
            _balance += amount;
        }
    }

    public void Withdraw(decimal amount)
    {
        lock (_transactionLock)
        {
            _balance -= amount;
        }
    }

    public void Save()
    {
        lock (_saveLock)
        {
            File.WriteAllText(@"C:\Balance.txt", _balance.ToString());
        }
    }
}

You'll notice that I have three locks, but only two variables.

The lines lock (_transactionLock) mutually lock the regions of code to only allow the current thread to enter - and this could mean that the current thread can re-enter the locked region. Other threads are blocked no matter which of the lock (_transactionLock) they hit if a thread already has the lock.

The second lock, lock (_saveLock), is there to show you that the object in the lock statement is there to identify the lock. So, if a thread were in one of the lock (_transactionLock) statements then there is nothing stopping a thread to enter the lock (_saveLock) block (unless another thread were already there).

Is it bad to overwrite a lock object if it is the last statement in the lock?

So, to start with, no the specific solution that you have provided is not safe, due to the specifics of how you're accessing the field.

Getting a working solution is easy enough. Just don't create a new list; instead, clear it out and add the new items:

class Foo
{
    private List<string> lockedList = new List<string>();

    public void ReplaceList(IEnumerable<string> items)
    {
        lock (lockedList)
        {
            lockedList.Clear();
            lockedList.AddRange(items);
        }
    }

    public void Add(string newItem)
    {
        lock (lockedList)
        {
            lockedList.Add(newItem);
        }
    }

    public void Contains(string item)
    {
        lock (lockedList)
        {
            lockedList.Contains(item);
        }
    }
}

Now your field isn't actually changing, and you don't need to worry about all of the problems that that can cause.

As for how the code in the question can break, all it takes is a call to Add or Contains to read the field, get the list, lock the list, and then have another thread replace the field. The when you read the field for a second time, after already getting the value to lock on, the value may have changed, so you'll end up mutating or reading from a list that another caller won't be restricted from accessing.

All that said, while mutating the lockedList variable is a really bad idea, and you should unquestionably avoid mutating it, as shown above, you can also ensure that you only actually read the field once, rather than reading from it repeatedly, and you'll still ensure that each list is only ever accessed from a single thread at any one time:

class Foo
{
    private volatile List<string> lockedList = new List<string>();

    public void ReplaceList(IEnumerable<string> items)
    {
        lockedList = new List<string>(items);
    }

    public void Add(string newItem)
    {
        var localList = lockedList;
        lock (localList)
        {
            localList.Add(newItem);
        }
    }

    public void Contains(string item)
    {
        var localList = lockedList;
        lock (localList)
        {
            localList.Contains(item);
        }
    }
}

Notice here that the problem that this fixes isn't mutating the field that the object to lock on was fetched from (that's not inherently the problem, although is a very bad practice), but rather constantly getting new values from the field in all usages of it from inside of the lock statements and expecting that value to never change, when it can.

This is going to be much harder to maintain, is very fragile, and is much more difficult to understand or ensure the correctness of though, so again, do do things like this.

Why Is Lock(This) {...} Bad