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!

Similar Posts:

Random Posts:

Recent Posts:

  • Mina Saad

    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

    [sourcecode language=”csharp”]Class Program
    {
    static void Main(string[] args)
    {

    Thread[] arr = new Thread[10];

    for (int i = 0; i < 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 < 10)
    {
    Thread.Sleep(1);
    Console.WriteLine(&quot;Thread #{0},t{1}&quot;,
    Thread.CurrentThread.ManagedThreadId, i++);

    }

    }
    }[/sourcecode]

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

  • Mina Saad

    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

    [sourcecode language=”csharp”]Class Program
    {
    static void Main(string[] args)
    {

    Thread[] arr = new Thread[10];

    for (int i = 0; i < 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 < 10)
    {
    Thread.Sleep(1);
    Console.WriteLine(&quot;Thread #{0},\t{1}&quot;,
    Thread.CurrentThread.ManagedThreadId, i++);

    }

    }
    }[/sourcecode]

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

  • 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:

    [sourcecode language=”csharp”]static void Main()
    {
    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();
    // the last line blocks the code until
    // that thread arr[i] finishes
    }
    }

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

    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:

    [sourcecode language=”csharp”] foreach (Thread t in arr)
    t.Join();[/sourcecode]

    with just this line:

    [sourcecode language=”csharp”] Console.ReadLine();[/sourcecode]

    or this:

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

  • 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:

    [sourcecode language=”csharp”]static void Main()
    {
    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();
    // the last line blocks the code until
    // that thread arr[i] finishes
    }
    }

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

    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:

    [sourcecode language=”csharp”] foreach (Thread t in arr)
    t.Join();[/sourcecode]

    with just this line:

    [sourcecode language=”csharp”] Console.ReadLine();[/sourcecode]

    or this:

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

  • Hey

    Really glad to get into this forum
    It’s what I am looking for.
    Hope to know more member here.

  • Hi,

    Thank you very much.

    Have you grabbed the RSS feed?

    Become a fan of Just Like a Magic in Facebook now:
    http://www.facebook.com/pages/Just-Like-a-Magic/109816769049047