The .Net frog

I've seen that kind of things in several codebases recently:

lock(this)
{
    // Do stuff...
}

Even if it perfectly works, this is a bad idea. You should never (or at least I can't think of a good reason!) lock on a public type, therefore including "this". There is a simple reason: you don't know what expectations the caller is doing.

Let's take a simple example. In the following code, we have a class that uses a lock on this (ClassThatLocksItself) and another class that is going to call it (CallerClass). When CallerClass calls ClassThatLocksItself, it's going to lock on the instance of ClassThatLocksItself.

    class Program
    {
        static void Main(string[] args)
        {
            ClassThatLocksItself myObj = new ClassThatLocksItself();
 
            CallerClass caller = new CallerClass(myObj);
            caller.LockTheObjectInAThread();
            Thread.Sleep(500);
 
            myObj.LockMe();
        }
    }
 
    class CallerClass
    {
        private ClassThatLocksItself _myObj;
        public CallerClass(ClassThatLocksItself myObj)
        {
            _myObj = myObj;
        }
 
        public void LockTheObjectInAThread()
        {
            ThreadPool.QueueUserWorkItem(LockTheObject);
        }
 
        public void LockTheObject(object state)
        {
            Console.WriteLine("Acquiring lock on the object");
            lock (_myObj)
            {
                Thread.Sleep(100000); // Do a long computation
            }
            Console.WriteLine("Releasing lock on the object");
        }
    }
 
    public class ClassThatLocksItself
    {
        public void LockMe()
        {
            Console.WriteLine("ClassThatLocksItself -- Trying to acquire lock on this");
            lock (this)
            {
                Console.WriteLine("ClassThatLocksItself -- lock on this acquired");
            }
            Console.WriteLine("ClassThatLocksItself -- lock on this released");
        }

If you try to execute this code, you'll notice that Console.WriteLine("ClassThatLocksItself -- lock on this acquired"); is only executed when LockTheObject() returns (here it takes 100 seconds). By using lock(this), you became dependant on externals and unknowns factors (in that case, the caller of your code decided to lock on your class). The situation is even a lot worse for developers who want to reuse your ClassThatLocksItself: they have no way of knowing that there can be a synchronisation problem unless they read the code of your class!

Now, try to guess what is going to happen if you change the implementation of LockTheObject with the following:

public void LockTheObject(object state)
        {
            Console.WriteLine("Acquiring lock on the object");
            lock (_myObj)
            {
                _myObj.LockMe(); // will not lock!
            }
            Console.WriteLine("Releasing lock on the object");
        }

Most of you will bet on a deadlock I guess. However, the CLR is intelligent enough to detect that when LockMe executes lock(this), the lock was already acquired by the caller. Therefore, it doesn't block on it. It makes the whole lock(this) thing very subtile: you'll only see a problem in some specific cases.