Threading - Never Lock This Redux | You’ve Been Haacked

A while ago I wrote that you should never lock a value type and never lock this. I presented a code snippet to illustrate the point but I violated the cardinal rule for code examples: compile and test it in context. Mea Culpa! Today in my comments, someone named Jack rightly pointed out that my example doesn’t demonstrate a deadlock due to locking this. As he points out, if the code were in a Finalizer, then my example would be believable.

To my defense, I was just testing to see if you were paying attention. ;) Nice find Jack!

My example was loosely based on Richter’s example in his article on Safe Thread Synchronization. Instead of rewriting his example, I will just link to it.

His example properly demonstrates the problem with a Finalizer thread attempting to lock on the object. However Jack goes on to say that locking on this in an ordinary method is fine. I still beg to differ, and have a better code example to prove it.

Again, suppose you carefully craft a class to handle threading internally. You have certain methods that carefully protect against reentrancy by locking on the this keyword. Sounds great in theory, no? However now you pass an instance of that class to some method of another class. That class should not have a way to use the same SyncBlock for thread synchronization that your methods do internally, right?

But it does!

In .NET, an object’s SyncBlock is not private. Because of the way locking is implemented in the .NET framework, an object’s SyncBlock is not private. Thus if you lock this, you are using to the current object’s SyncBlock for thread synchronization, which is also available to external classes.

Richter’s article explains this well. But enough theory you say, show me the code! I will demonstrate this with a simple console app that has a somewhat realistic scenario. Here is the application code. It simply creates a WorkDispatcher that dispatches a Worker to do some work. Simple, eh?

class Program
{
    static void Main()
    {
        WorkDispatcher dispatcher = new WorkDispatcher();
        dispatcher.Dispatch(new Worker());
    }
}

Next we have the carefully crafted WorkDispatcher. It has a single method Dispatch that takes a lock on this (for some very good reason, I am sure) and then dispatches an instance of IWorker to do something by calling its DoWork method.

public class WorkDispatcher
{
    int dispatchCount = 0;
    
    public void Dispatch(IWorker worker)
    {
        Console.WriteLine("Locking this");
        lock(this)
        {
            Thread thread = new Thread(worker.DoWork);
            Console.WriteLine("Starting a thread to do work.");
            dispatchCount++;
            Console.WriteLine("Dispatched " + dispatchCount);
            thread.Start(this);
            
            Console.WriteLine("Wait for the thread to join.");
            thread.Join();
        }
        Console.WriteLine("Never get here.");
    }
}

From the look of it, there should be no reason for this class to deadlock in and of itself. But now let us suppose this is part of a plugin architecture in which the user can plug in various implementations of the IWorker interface. The user downloads a really swell plugin from the internet and plugs it in there. Unfortunately, this worker was written by a malicious eeeeevil developer.

public class Worker : IWorker
{        
    public void DoWork(object dispatcher)
    {
        Console.WriteLine("Cause Deadlock.");
        lock (dispatcher)
        {
            Console.WriteLine("Simulating some work");
        }
    }
}

The evil worker disrupts the carefully constructed synchronization plans of the WorkDispatcher class. This is a somewhat contrived example, but in real world multi-threaded application, this type of scenario can quite easly surface.

If the WorkDispatcher was really concerned about thread safety and protecting its synchronization code, it would lock on something private that no external class could lock on. Here is a corrected example of the WorkDispatcher.

public class WorkDispatcher
{
    object syncBlock = new object();
    int dispatchCount = 0;
    
    public void Dispatch(IWorker worker)
    {
        Console.WriteLine("Locking this");
        lock (this.syncBlock)
        {
            Thread thread = new Thread(worker.DoWork);
            Console.WriteLine("Starting a thread to do work.");
            dispatchCount++;
            Console.WriteLine("Dispatched " + dispatchCount);
            thread.Start(this);
            
            Console.WriteLine("Wait for the thread to join.");
            thread.Join();
        }
        Console.WriteLine("Now we DO get here.");
    }
}

So Jack, if you are reading this, I hope it convinces you (and everyone else) that locking on this, even in a normal method, is a pretty bad idea. It won’t always lead to problems, but why risk it?

Comments

6 responses

jayson knight • August 8th, 2006
Wouldn't it make more sense to use the Monitor class?
Haacked • August 8th, 2006
Huh? Well the lock statement uses the monitor class under the hood.
It would make more sense to use the TimedLock class.
However, even so, you would force a thread lock timeout if you lock on this.
jayson knight • August 8th, 2006
As always I get to look like an amateur ;-). You are correct, I've just always been told to A) not use the lock statement and B) use the Monitor (or related) class instead.
I love learning new stuff.
Haacked • August 8th, 2006
Ah, I see. Well personally, I never use the lock statement as is because it does not specify a timeout. Instead, I use the TimedLock because it provides the best of both worlds.
It has lock-like semantics (via a using block), but allows you to specify a timeout (like using a monitor).
The problem with using a monitor directly is you need to remember to always write the exit code properly by putting it in a try/finally block.
And "try/finally" always triggers "using!" in my head when applicable.
SepzonE • August 22nd, 2006
In general, you should create a new thread "manually" for long-running tasks. For example ...ThreadStart
Ada Lovelace • December 9th, 2015
I have had this argument with coworkers and on stack overflow many times. I therefore dedicated an article in my own blog to state why lock(this) is totally fine and appropriate among other things that they tell you not to do.

There's a huge, huge flaw in your code there. Why on earth are you holding lock(this) while waiting on a thread.join() that is also wanting a lock on that object? THAT IMO is the bad practice. The code is basically doing exactly what you told it to do, you created a thread, passed it an object, and blocked it from actually being able to do work. This is the same thing that would happen if in C++ you passed a critical section object to another thread that you forgot to unlock.

Deadlocks happen if you're not careful, but so far no soul has actually demonstrated to me a situation where lock(this) is actually the real source of any problem. And my blog demonstrates reasons and scenarios where calling lock(this) is actually far less likely to introduce deadlocks than locking internal objects.

So far no one has shown me a substantive argument against using lock(this). Were you really expecting the worker thread to operate properly even though you held a lock on it throughout its entire execution time? This argument seems more about design semantics and IMO, when the Microsoft documentation says that you "shouldn't do something", it really should be based on fact, not philosophy.

Anyway, you can argue that calling lock(this) was "bad form" in your example, or you could argue that holding a lock on data that another thread needs to do its work is bad form. Regardless, shared data needs to be locked sometimes and it really doesn't matter how it gets locked, as long as you know what you're doing and plan accordingly.