Posted on in Threading

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

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!

11 thoughts on “Bad Practices: Locking on Non-shared Objects in Multi-threaded Applications”

  1. Thanks for that topic but I have a small comment , you can the same as above without using lock(obj) as the ThreadObject.Join() will do that for you check it on dotnetspark (http://www.dotnetspark.com/qa/339-what-is-threadjoin-threading.aspx) but you must run it after the thread start directly

    Class Program
{
static void Main(string[] args)
        {

            Thread[] arr = new Thread[10];

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

            }
}
static void ThreadProc()
        {

                int i = 0;
                while (i &lt; 10)
                {
                    Thread.Sleep(1);
                    Console.WriteLine(&amp;quot;Thread #{0},t{1}&amp;quot;,
                        Thread.CurrentThread.ManagedThreadId, i++);

                }



        }
}

    Please confirm me if I'm doing something wrong , thanks for that cool information ,keep it up

    Like

  2. Thanks for that topic but I have a small comment , you can the same as above without using lock(obj) as the ThreadObject.Join() will do that for you check it on dotnetspark (http://www.dotnetspark.com/qa/339-what-is-threadjoin-threading.aspx) but you must run it after the thread start directly

    Class Program
{
static void Main(string[] args)
        {

            Thread[] arr = new Thread[10];

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

            }
}
static void ThreadProc()
        {

                int i = 0;
                while (i &lt; 10)
                {
                    Thread.Sleep(1);
                    Console.WriteLine(&amp;quot;Thread #{0},\t{1}&amp;quot;,
                        Thread.CurrentThread.ManagedThreadId, i++);

                }



        }
}

    Please confirm me if I'm doing something wrong , thanks for that cool information ,keep it up

    Like

  3. No dear, It’s not like that. This is not called synchornization. Thread.Join() blocks the calling thread until that thread finishes. Consider the following example:

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

    for (int i = 0; i &amp;lt; 10; i++)
    {
        arr[i] = new Thread(ThreadProc);
        arr[i].IsBackground = true;
        arr[i].Start();
        arr[i].Join();
        // the last line blocks the code until
        // that thread arr[i] finishes
    }
}

private static void ThreadProc(object obj)
{
    int i = 0;
    while (i &amp;lt; 10)
    {
        Thread.Sleep(1);
        Console.WriteLine(&amp;quot;Thread #{0},t{1}&amp;quot;,
        Thread.CurrentThread.ManagedThreadId, i++);
    }
}

    Now, only one thread accesses the ThreadProc procedure because each time the loop spawns only one thread and waits until that thread finishes. Therefore, ThreadProc is a thread-safe function because it’s accessed only by one thread at a moment, and no synchronization is needed.

    Actually, this line just blocks the application from exiting until all threads finishes. Therefore, you can replace this loop:

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

    with just this line:

        Console.ReadLine();

    or this:

            arr[i].IsBackground = false; // switching it to a foreground thread

    Like

  4. No dear, It’s not like that. This is not called synchornization. Thread.Join() blocks the calling thread until that thread finishes. Consider the following example:

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

    for (int i = 0; i &amp;lt; 10; i++)
    {
        arr[i] = new Thread(ThreadProc);
        arr[i].IsBackground = true;
        arr[i].Start();
        arr[i].Join();
        // the last line blocks the code until
        // that thread arr[i] finishes
    }
}

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

    Now, only one thread accesses the ThreadProc procedure because each time the loop spawns only one thread and waits until that thread finishes. Therefore, ThreadProc is a thread-safe function because it’s accessed only by one thread at a moment, and no synchronization is needed.

    Actually, this line just blocks the application from exiting until all threads finishes. Therefore, you can replace this loop:

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

    with just this line:

        Console.ReadLine();

    or this:

            arr[i].IsBackground = false; // switching it to a foreground thread

    Like

Leave a reply to Mina Saad Cancel reply