Bad Practices: Locking on Non-shared Objects in Multi-threaded Applications

This article is also available in my blog, Just Like a Magic.

Actually, I was having a problem synchronizing threads calling a function. If we could regenerate the bug, we would end up with code like this:

static void Main()
{
    Thread[] arr = new Thread[10];

    for (int i = 0; i < 10; i++)
    {
        arr[i] = new Thread(ThreadProc);
        arr[i].IsBackground = true;
        arr[i].Start(new object());
    }

    foreach (Thread t in arr)
        t.Join();
}

private static void ThreadProc(object obj)
{
    lock (obj)
    {
        int i = 0;
        while (i < 10)
        {
            Thread.Sleep(1);
            Console.WriteLine("Thread #{0},\t{1}",
                Thread.CurrentThread.ManagedThreadId, i++);
        }
    }
}

And when we execute this code the results would be like this:

Thread #4,      2
Thread #3,      3
Thread #5,      1
Thread #7,      0
Thread #5,      2
Thread #6,      1
Thread #3,      4
Thread #4,      3
Thread #8,      1
Thread #9,      0

What is the problem with this code? It starts multiple threads and passes them a locking object and the object is locked successfully using the lock statement, so why threads overlap? Why the synchronization doesn't take effect?

Well, after a long period and after pushing a new thread in the MSDN forums (we are all developers do make silly mistakes, aih? :P), I come up with the solution and discovered the drawback of the code.

The problem was that each time we start off a thread we pass it a new locking object different from the others:

arr[i].Start(new object());

Therefore, every thread is locking on its own objects, so no thread synchronization take effect.

The solution is very easy, you should lock on a shared object; an object that is shared between all your threads accessing this block of code. Read more about thread synchronization here.

For example, we should change our code to the following:

private static object _lockThis = new object();
static void Main()
{
    Thread[] arr = new Thread[10];

    for (int i = 0; i < 10; i++)
    {
        arr[i] = new Thread(ThreadProc);
        arr[i].IsBackground = true;
        arr[i].Start();
    }

    foreach (Thread t in arr)
        t.Join();
}

private static void ThreadProc(object obj)
{
    lock (_lockThis)
    {
        int i = 0;
        while (i < 10)
        {
            Thread.Sleep(1);
            Console.WriteLine("Thread #{0},\t{1}",
                Thread.CurrentThread.ManagedThreadId, i++);
        }
    }
}

Finally, this was one of the bad practices and mistakes me (and many others too) fall in. Honestly, I would like to start my Bad Practices series :">. I would write about problems I came across and solutions I found for them. In addition, I'm thinking of starting the Questions series. I got a huge number of questions every week, if we could publish them I think it would be great for all.

Have a nice day!