Tuesday, June 11, 2013

C# - Why you must never lock on this

You should never lock on “this” if this points to an instance of a publicly accessible class. The reason is that if the class is publicly accessible, then you have no control over whether someone else who uses your class, uses the instance for locking. And if they do lock an instance of your class, then you will end up in a dead-lock.

Here is some simple code to illustrate this:

using System.Threading.Tasks;

void Main()
{
    ClassTest test = new ClassTest();
    lock(test) //locking on the instance of ClassTest
    {
        Parallel.Invoke (new Action[]{() => test.DoWorkUsingThisLock(1)});
    }
}

public class ClassTest
{
    public void DoWorkUsingThisLock(int i)
    {
        Console.WriteLine("Before ClassTest.DoWorkUsingThisLock " + i);
        lock(this) //this is bad - this will never end - you have been deadlocked!
        {
            Console.WriteLine("ClassTest.DoWorkUsingThisLock " + i);
            Thread.Sleep(1000);
        }
        Console.WriteLine("ClassTest.DoWorkUsingThisLock Done " + i);
    }
}

From MSDN:

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.

References:

Lock Statement (MSDN)

No comments: