Reader Level:
Article

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

By Mohammad Elsheimy on Apr 24, 2010
In this article we will see one of the bad practices developers always do.

This article was previously published 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!

Mohammad Elsheimy

Independent software developer, trainer, and technical writer from Egypt. Student in Holy Quranic Sciences college, Azhar University.Blogs- http://JustLikeAMagic.com- http://WithDotNet.netFollow me @elsheimy on Twit... Read more

Personal Blog: http://JustLikeAMagic.com

COMMENT USING