Threading modifying unintended variable

Question

I'm fairly new to C# threading, so apologies in advance for this newbie error. The aim of the following code is to evaluate the fitness of a population that is stored in an array called members. The procedure members.calculateFitness() is a black box (i.e. I can't modify it to improve performance), so I'm trying to setup threads that call the black box simultaneously, and each thread will deal with 1/THREAD_COUNT population members (so if THREAD_COUNT=4, each thread will deal with 1/4 of the population).

In the first for loop I initialise each thread. In the second for loop, I start the thread.

public void ThreadedPrintPopFitness ()
    {
        int THREAD_COUNT = 1;
        int membersPerThread = members.Length / THREAD_COUNT;

        Thread[] fitnessCalculator = new Thread[THREAD_COUNT];
        int[] threadResult = new int[THREAD_COUNT];

        for (int i = 0; i < THREAD_COUNT; i++) {
            int start = i * membersPerThread;
            int stop = (i+1) * membersPerThread;
            fitnessCalculator [i] = new Thread (() => getMaxFitness (members, start, stop, ref threadResult [i]));
        }

        for (int i = 0; i < THREAD_COUNT; i++) {
            fitnessCalculator [i].Start ();
        }

        for (int i = 0; i < THREAD_COUNT; i++) {
            fitnessCalculator [i].Join ();
        }

        int maxFitness = 0;
        for (int i = 0; i < THREAD_COUNT; i++) {
            if (maxFitness < threadResult [i])
                maxFitness = threadResult [i];
        }

        Console.WriteLine ("(ThreadedCount) Fittest Population Member's Fitness: " + maxFitness);
    }

    private static void getMaxFitness (PopulationMember[] members, int start, int stop, ref int result)
    {
        int maxFitness = 0;

        for (int i = start; i < stop && i < members.Length; i++) {
            if (members [i].calculateFitness () > maxFitness) {
                maxFitness = members [i].lastFitness;
            }
        }

        result = maxFitness;
    }

Stepping through the code shows that it gets into the second for loop, and then jumps back to the first for loop and declares an IndexOutOfBoundsException on the integer i. I can see that i = THREAD_COUNT (I've tried with different numbers for THREAD_COUNT).

I'm completely baffled, what am I doing wrong? Thanks in advance!

enter image description here

You should look into the [Task](http://msdn.microsoft.com/en-us/library/system.threading.tasks.task%28v=vs.110%29.aspx) class rather than creating your own threads in this manner. `Task` uses a threadpool, which is what you'll want in 99% of cases. — MgSam, Dec 10 '13 at 21:06
@Servy How is this a possible duplicate when the poster has no idea what the problem was? It's not like he asked "why can't I close over a loop variable". You're just encouraging new users not to use the site by being so aggressive about duplicates when someone doesn't even understand the problem. — MgSam, Dec 10 '13 at 21:07
@MgSam Just because the author doesn't know that it's a duplicate doesn't make it not a duplicate. Why *shouldn't* you close the question as a duplicate when it's the same? Answering the same question over and over again, even if the authors of each were oblivious that it was a duplicate, is not the appropriate course of action. Closing the question as a duplicate points the OP to the exact problem. How is having their problem solved discouraging them from asking questions? — Servy, Dec 10 '13 at 21:15
@Servy It is *not* the same question just because the bug ultimately turned out to be the same. It can be bewildering for someone inexperienced to try to find the bug in a non-trivial amount of code. You expect them to be able to instantly understand the issue just because you paste a link to a "duplicate" question? If you extend that logic, you should just close every question and provide a link to the C# language spec. After all, a good deal of this site is just duplicating what's written in there. — MgSam, Dec 10 '13 at 21:21
@MgSam `"You expect them to be able to instantly understand the issue just because you paste a link to a "duplicate" question?"` Well, the user *did* instantly understand the problem just because I linked to the duplicate question, so...yes. It's someone that has pretty much the exact same problem, and the answer explains the problem and the solutions in great detail. Reading through the question is exactly what you should be doing to understand this particular problem. And questions that can be very clearly answered by simply reading the C# language spec often *should* be closed. — Servy, Dec 10 '13 at 21:25

score 0 · Answer 1 · answered Dec 10 '13 at 20:34

0

Servy has it spot on, I even remember reading about this in John Skeet's book now. I had to make a copy of i within the for loop, this fixed it.

Thanks!

answered Dec 10 '13 at 20:34

Thomas Wormald

187
10

score 0 · Answer 2 · answered Dec 10 '13 at 20:35

As written in the comments, i is captured, and when increased, the function refers to the new value.

What you should do is copy its value to a local variable:

for (int i = 0; i < THREAD_COUNT; i++) {
      int start = i * membersPerThread;
      int stop = (i+1) * membersPerThread;
      int resultId = i;
      fitnessCalculator [i] = new Thread (() => getMaxFitness (members, start, stop, ref threadResult [resultId]));
}

Threading modifying unintended variable

2 Answers2