c# Lock Doesn`t work / Critical Section

Question

I`m trying to advance a static (int) counter using two different threads in a FOR loop, so if the loop is runs 10 times I (should) get counter=20. for some reason i keep getting different values each time i run the loop (19,20,21), even though i used a LOCK to access that counter, (the code runs in console):

public static int Counter = 0;
static object syncObject = new object();

static void Main(string[] args)
        { 

int ForLength = 10;

            Thread FirstThread, SecondThread;

            for (int i = 0; i <= ForLength; i++)
            {
                FirstThread = new Thread(RaiseCounter);
                FirstThread.IsBackground = false;

                SecondThread = new Thread(RaiseCounter);
                SecondThread.IsBackground = false;               


                FirstThread.Start();                
                SecondThread.Start();

                //Console.WriteLine(Counter);
            }


            Console.WriteLine(Counter);

            Console.ReadLine();
        }

        public static void RaiseCounter ()
        {
            lock (syncObject)
            {
                Counter++;
            }
        }

You are starting 22 threads but you never wait for them to complete. Also you need to use the lock when reading `Counter` as well. — Mike Zboray, Mar 23 '15 at 18:43

Jon Skeet · Accepted Answer · 2015-03-23T18:44:39.277

10

You've got three problems:

You're actually running the loop 11 times:

for (int i = 0; i <= ForLength; i++) // i=0 to i=10 *inclusive*

You're not joining on your threads (or sleeping), so some of them may not have finished by the time you write the output
You're not synchronizing on syncObject when you read Counter in the main thread, so you may not observe the most recently written value

If you don't want to use Thread.Join, just add a call to Thread.Sleep, e.g. Thread.Sleep(5000) - it's extremely likely that all the threads will have completed after that. Then you can use:

lock (syncObject)
{
    Console.WriteLine(Counter);
}

In short, there's nothing wrong with lock, although you'd be better off using Interlocked.Increment.

edited Mar 23 '15 at 18:44

answered Mar 23 '15 at 18:42

Jon Skeet

1,421,763
867
9,128
9,194

The power of idioms... I saw the join issue too but was struggling to explain the output of 21, until you pointed out the `<=`. – Eric J. Mar 23 '15 at 18:44
@Jon, adding FirstThread .Join() didnt seem to change the outcome - different values each time i run the look. – Ran Mar 23 '15 at 18:50
Also, I prefer to avoid using Thread.Sleep(), as the loop will run many more times (100,000), is there any other solution? – Ran Mar 23 '15 at 18:51
1

@Ran: You'd need to join on *all* the threads you create. And it sounds like there's context you haven't shown us... but you really don't want to be creating 100,000 threads anyway. I'd advise you to use the Task Parallel Library. However, all of that is outside the scope of this question, which is about why the code you provided didn't work - and I've explained that. If there's more to it, I suggest you ask a *new* question with the additional information – Jon Skeet Mar 23 '15 at 18:53
@Jon, this is an exercise (of sort) where i specifically need to use threads, I have shorten the loop length to 10 for explaining perpose (my english is crappy as it is), my main problem is: why do i keep getting different results each time i run the loop? (also, how do I join ALL threads?) – Ran Mar 23 '15 at 18:58
@Ran: You get different results each time you run it because it depends on how many threads have completed before you print the result. And if you want to join all the threads, you'd need to keep a collection of threads, e.g. a `List` and iterate over all of them. – Jon Skeet Mar 23 '15 at 19:11

c# Lock Doesn`t work / Critical Section

1 Answers1