CODEWITHSUNDEEP
c#thread-safety
Domi.Zhang
Domi.Zhang

Reputation: 37

Why "lock" block not work?

public class ThreadInteroperateWithLock
{
    private int m_count;
    private object m_synLock;
    public ThreadInteroperateWithLock() 
    {
        m_count = 0;
        m_synLock = new object();
    }

    public int Count { get { return m_count; } }

    public void Add() 
    {
        //just simulate some work
        int temp=0;
        for (int i = 0; i < 10000; i++)
        {
            temp++;
        }

        //really job
        lock (m_synLock)
        {
            m_count++;
        }
    }
}

This code is in a console application:

ThreadInteroperateWithLock ope = new ThreadInteroperateWithLock();
Thread[] threadArray = new Thread[100];
for (int i = 0; i < 100; i++)
{
    Thread thread = new Thread(new ThreadStart(ope.Add));
    thread.IsBackground = false;
    threadArray[i] = thread;
}
for (int i = 0; i < 100; i++)
{
    threadArray[i].Start();
}
Console.WriteLine(ope.Count);
Console.ReadKey();

Sometime it prints '99' and sometime '100', regardless of whether the lock{...} block exists or not. Is there any wrong in my code?

Upvotes: 2

Views: 135

Answers (7)

Grant Thomas
Grant Thomas

Reputation: 45058

The problem here would be that you're kicking off the threads and they're not entirely finished by the time of your call to write output to the console.

The loop starts off i threads, we'll imagine these as bees going of collecting as work, not all going to the same food source so some take longer to return than others; then, back at the hive, we suddenly say: "Hey, bees, I need a headcount!", "...1, 2, 3..., only three?" No, some i-3 are still scrounging around outside!

So, the idea is that we must have an indicator of when work is completed, or a homing signal of sorts to get all bees back to the hive for the headcount. This can be done with Join, or manual state checking (which essentially has you holding out until the last sojourner returns.)

Upvotes: 4

Isak Savo
Isak Savo

Reputation: 35904

When you print the count you have no idea of whether or not the threads have completed or not. A "correct" output in your code could be any number between 0 and 100.

To be certain to get 100 as output, you mustn't get the value of Count until all threads have completed. You can do that by calling Thread.Join on all threads or (even better) use something like Parallel.For to start the threads:

Parallel.For(0, 100, idx => ope.Add);
// When we reach this line, we know all threads have completed so we can 
// safely get the count
var count = ope.Count;

Upvotes: 0

spender
spender

Reputation: 120498

It's a classic race condition, and I consider you lucky to get anywhere near 100 by the time your Console.WriteLine is invoked.

for (int i = 0; i < 100; i++)
{
    threadArray[i].Start();
}
//absolutely no guarantee that **ANY** threads have completed before next line
Console.WriteLine(ope.Count);

Consider using a CountdownEvent or similar to do a bit of synchronization.

Upvotes: 1

Stephan
Stephan

Reputation: 4247

It is not deterministic when your call to Count happens. It may happen before any thread has finished, or after all threads have finished or even somewhere in the middle. If you want to wait until all thread have finished, you should join them.

Upvotes: 1

AakashM
AakashM

Reputation: 63358

You're not waiting for the threads to finish. It'e entirely a matter of luck how many have done their work before you output the count.

Add

        for (int i = 0; i < 100; i++)
        {
            threadArray[i].Join();

        }

before the WriteLine, and you always get 100.

Upvotes: 3

Tony The Lion
Tony The Lion

Reputation: 63250

You should protect your getter public int Count { get { return m_count; } } with a lock to, else a thread B could be reading that value, while another thread A is updating count in your Add method, this can cause you to get an inconsistent view of your data.

Upvotes: 1

Jamiec
Jamiec

Reputation: 136134

Not sure that this is the answer you're looking for, but to incrememnt a counter in a threadsafe way you should use Interlocked.Increment docs

Upvotes: 0

Related Questions