C# basic multithreading issue - How to use locks to solve race conditions

Question

public class ThreadingIssue
{
    int accum;

    public void Squaring(int x)
    {
        // Creates a random pause to check for thread
        int pauseFor = rnd.Next(1, 10);
        Thread.Sleep(pauseFor);

        accum += x*x;

     }

     public void DoSquaring()
     {
       accum = 0;
       for (int i = 1; i <= 100; i++)
       {
          threads.Add(new Thread(() => Squaring(i)));
          threads[i - 1].Start();
       }
      }

Essentially I wanted to get 338350 as the answer every time, but obviously I do not as there is a threading issue. I have tried to create an object like:

 private System.Object lockThis = new System.Object();

and then lock the accum variable like so:

 lock (lockThis)
 {
    accum += x*x;
 }

But this does not fix the issue. What am I missing/doing wrong?

Answer 1

I know this isn't strictly what was asked, but in the interest of steering you in the right direction, this task is perfectly suited for very minimal Linq and Tasks.Parallel:

var bag = new ConcurrentBag<KeyValuePair<int, int>>();

Parallel.For(1, 101, 
    i => 
    {
        bag.Add(new KeyValuePair<int, int>(i, i * i));
    });

var squares = bag.ToDictionary(pair => pair.Key, pair => pair.Value);

Console.WriteLine("Square of 56 is " + squares[56].ToString());
Console.WriteLine("Sum of all squares is " + squares.Sum(pair => pair.Value).ToString());

This takes advantage of the relatively new System.Collections.Concurrent namespace to completely remove the need for managing locks manually, and also of the System.Threading.Tasks namespace to parallelize a workload. The loop fills in the collection using many threads, letting the OS decide how many threads it will spawn to keep the system responsive, and you can do whatever you want with it after it's been cast into a dictionary. You can also pass in a ParallelOptions instance to the Parallel.For() call to specify if the task is short or long running to tell the OS how it should prioritize it, and to specify the max degree of parallelism for the task.

Just make sure you don't use Tasks.Parallel on your form's primary thread.

Answer 2

First, do this:

public class ThreadingIssue
{
    int accum;
    private readonly object _syncLock = new object();

    public void Squaring(int x)
    {
        // Creates a random pause to check for thread
        int pauseFor = rnd.Next(1, 10);
        Thread.Sleep(pauseFor);

        lock( _syncLock ) 
        {
           accum += x*x;
        }
     }

     public void DoSquaring()
     {
       accum = 0;
       for (int i = 1; i <= 100; i++)
       {
          int number = i;
          threads.Add(new Thread(() => Squaring(number)));
          threads[i - 1].Start();
       }
      }
    }

Answer 3

Your for loop is at fault.

for (int i = 1; i <= 100; i++)
{
    threads.Add(new Thread(() => Squaring(i)));
    threads[i - 1].Start();
}

The loop variable i is used inside the lambda () => Squaring(i) when starting each thread, but by the time any thread is created the loop has finished and the value of i is 101.

To fix this you need to capture a copy of i in the loop and use that.

Try this:

for (int i = 1; i <= 100; i++)
{
    int j = i;
    threads.Add(new Thread(() => Squaring(j)));
    threads[i - 1].Start();
}

You also need to lock around the accum += x * x; line.

private object gate = new object();
public void Squaring(int x)
{
    // Creates a random pause to check for thread
    int pauseFor = rnd.Next(1, 10);
    Thread.Sleep(pauseFor);

    lock (gate)
    {
        accum += x * x;
    }
}