Thread alters passed Int, if start() is called separately

Question

I've read some things on threading and know that I should lock up a variable that is being accessed by multiple threads. The problem that I am experiencing is that the value in MainQueueToWorkingQueue(x) is always three if I kick off the thread by first allocating the thread, then calling start in the second for loop.

But, if I do this new Thread(() => MainQueueToWorkingQueue(x)).Start(); then the code runs as expected and the correct number is passed.

        private static List<BlockingQueue> WorkingQueueList = new List<BlockingQueue>();
static void Main(string[] args)
{
        for(Int32 WorkingQueueListInsers = 0; WorkingQueueListInsert < 3; WorkinfQueueListInsert++)
        {
           WorkingQueueList.Add(new BlockingQueue(20));
        }
        Thread[] MainThreads = new Thread[WorkingQueueList.Count];

        for (Int32 x = 0; x < WorkingQueueList.Count; x++)
        {
            /* MainThreads[x] = */ new Thread(() => MainQueueToWorkingQueue(x)).Start();
            Thread.Sleep(50);
        }

        for (Int32 x = 0; x < WorkingQueueList.Count; x++)
        {
            MainThreads[x].Start();
            Thread.Sleep(50);
        }
        Console.Read();
    }


    private static void MainQueueToWorkingQueue(Int32 WorkingQueuePosition)
    {
        /* if new Thread(() => MainQueueToWorkingQueue(x)).Start(); is called
             then WorkingQueuePosition is correct and is either zero, one, or two. */

        /* if the thread is allocated in one for loop, then started in another for loop,  WorkingQueuePosition only equals three */
        Console.WriteLine("Ending x: " + WorkingQueuePosition);
        while (true)
        {
            WorkingQueueList[WorkingQueuePosition].Enqueue(MainQueue.Dequeue());
        }
    }

My question is this. Why is the passed parameter correct when I use Start() when making the new thread, but the passed parameter is always three when I call Start() in a second for loop?

My guess: I know somewhere the parameter is being altered. My first guess was that the loop was running to fast that the thread used a different value than the one passed because x was updated too quickly. I tried fixing that with a Thread.Sleep(50) but the issue still remained.

EDIT: Took out code that did not deal directly with the problem.

possible duplicate of [C# Captured Variable In Loop](http://stackoverflow.com/questions/271440/c-sharp-captured-variable-in-loop) — shf301, Nov 14 '13 at 15:08
You've provided a lot of unnecessary code here. Please slim down the code to *only* show what it needs to show. I'm pretty sure this is just a matter of lambda expressions capturing the *variable* (rather than the value) but it's hard to tell with the extra baggage you've got here. — Jon Skeet, Nov 14 '13 at 15:09
@JonSkeet Pardon me for saying this, but I thought I had slimmed down my code. My only guess on how to slim down the code is to only have the code that is in question. So the calls and the method body of WorkingQueueToJob() should be deleted. Also the Console.WriteLines()'s should be deleted. Is that right? Or am I wrong? I want to know so I can ask more clarifying questions in the future. — Darren Hoehna, Nov 14 '13 at 15:39
@DarrenHoehna This (http://sscce.org/) is a good explanation of what your code sample should be. — T. Kiley, Nov 14 '13 at 15:42
@DarrenHoehna: There should be *just* enough code to reproduce the problem - but it should be complete. Currently it's both incomplete and overly long; the bodies of both `WorkingQueueToJob` and `MainQueueToWorkingQueue` methods do more than they need to, and `WorkingQueueList` isn't really needed. You say that in the example the count is 3, but nothing sets it up, etc... — Jon Skeet, Nov 14 '13 at 15:50
@JonSkeet I see what you are saying. I looked at sscce.org and tried to implement some of their guide lines. I have booked marked that site and will use it for future reference. — Darren Hoehna, Nov 14 '13 at 15:57
@T.Kiley Thank you for that resource. I have the site book marked and will use it in the future. — Darren Hoehna, Nov 14 '13 at 15:58

Iridium · Accepted Answer · 2013-11-14T15:18:43.767

Your problem is in the use of lambda expressions here where you are forming a closure over the loop variable x, not its value at that point. As a result, when the threads actually start, the value of x may have changed (because the loop has executed further iterations) and it is the new value that is passed to your methods.

If you are using ReSharper, it will warn you of an "Access to modified closure".

Note that your "working" version is still vulnerable the problem, but as the threads are started in the loop, there's a greater chance that the value of x won't have changed (especially with your 50ms sleep in there). However if one of the threads takes > 50ms to start up, you will still see the wrong value.

You can fix this by copying the value of x to a local variable inside the loop. This will fix the code in both cases - whether you start the threads in this loop, or store the threads into the MainThreads/WorkingThreads arrays and start them later. Your 50ms sleep should also no longer be required.

    for (Int32 x = 0; x < WorkingQueueList.Count; x++)
    {
        var localX = x;
        Console.WriteLine("Starting x: " + x);
        /* MainThreads[x] = */ new Thread(() => MainQueueToWorkingQueue(localX)).Start();
        /* WorkingThreads[x]  =*/ new Thread(() => WorkingQueueToJob(localX)).Start();
    }

You can read more about this issue here: http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx

Thread alters passed Int, if start() is called separately

1 Answers1

Linked

Related