Cannot understand the behaviour of the threads C#

Question

    static void Main(string[] args)
    {
            var sw = new Stopwatch();
            sw.Start();

            int noOfThreads = Environment.ProcessorCount;
            //int minVal = 1;
            int maxVal = 10000000;
            int  blockSize = maxVal / noOfThreads;
            List<Thread> threads = new List<Thread>();
            List<List<int>> results = new List<List<int>>();
            object thisLock = new object();

            for (int i = 0; i < noOfThreads; ++i)
            {
                lock(thisLock)
                {
                    Thread th = new Thread(() =>
                    {
                        results.Add(GetPrimeNumbers(i * blockSize, i * blockSize + blockSize));
                    });

                    th.Start();
                    threads.Add(th);
                }
            }

            foreach (var elem in threads)
                elem.Join();
   }

   private static List<int> GetPrimeNumbers(int low, int high)
   {
        List<int> result = new List<int>();
        //Debug.WriteLine("Low: {0}. High: {1}", low, high);
        for(int i = low; i <= high; ++i)
        {
            if (IsPrime(i))
                result.Add(i);
        }

        return result;
    }

    static bool IsPrime(int number)
    {
        if (number % 2 == 0)
            return false;
        else
        {
            var topLimit = (int)Math.Sqrt(number);
            for (int i = 3; i <= topLimit; i += 2)
                if (number % i == 0) 
                    return false;

            return true;
        }
    }

With the above code, I was expecting that when I put breakpoint in the GetPrimeNumbers(int low, int high) I would see range of values for low and high, e.g: (0, 1250000), (1250000, 2500000).....(8750000, 10000000). But what I observing is that there are certain blocks that gets passed multiple times - (2500000, 3750000) while certain do not passed at all -(0, 1250000) and this behaviour also matches the results I am getting.

I am curious why I am seeing this behaviour. Is there a way to prevent this?

I am aware of the fact that I can use Parallel.For() and over here I do see the expected behaviour at breakpoint in GetPrimes(int low, int high). But as I mentioned before I am curious why I am seeing the former behaviour.

Thanks in advance!

Answer 1

The problem is that a for loop reuses the same i variable across iterations, and your thread delegate is closing over that variable.

There are various ways to fix this. A simple one is to use a new variable declared within your loop:

for (int i = 0; i < noOfThreads; ++i)
{
    int j = i; // capture the value
    lock(thisLock)
    {
        Thread th = new Thread(() =>
        {
            results.Add(GetPrimeNumbers(j * blockSize, j * blockSize + blockSize));
        });

        th.Start();
        threads.Add(th);
    }
}

This still has other issues, though. I'd recommend something more like this:

var allPrimeNumbers = Enumerable.Range(0, numberOfThreads)
    .AsParallel()
    .SelectMany(i => GetPrimeNumbers(i * blockSize, i * blockSize + blockSize))
    .ToList();

Further Reading
Is there a reason for C#'s reuse of the variable in a foreach?

Answer 2

StriplingWarrior had it close, but as mentioned in the comments, you still have a threading bug. You need to move the lock inside the Thread action. Also, to get the best performance, hold the lock for the shortest amount of time possible, which is when modifying the shared results variable. To do that I separated the GetPrimeNumbers call from the results.Add call.

for (int i = 0; i < noOfThreads; ++i)
{
    int j = i; // capture the value
    Thread th = new Thread(() =>
    {
        result = GetPrimeNumbers(j * blockSize, j * blockSize + blockSize);
        lock(thisLock) 
        {
           results.Add(result);
        }
    });

    th.Start();
    threads.Add(th);
}

Also, unless you really need to manage your own threads I would recommend using Tasks (TPL) instead. Here is a modification using Tasks

Task<List<int>> tasks = new Task<List<int>>();
for (int i = 0; i < noOfThreads; ++i)
{
    int j = i; // capture the value
    tasks.Add(Task.Run(() => GetPrimeNumbers(j * blockSize, j * blockSize + blockSize)));
}

Task.WaitAll(tasks);
results = tasks.Select(t => t.Result).ToList();