Starting a new thread in a foreach loop

Question

I have a List of objects and I'd like to loop over that list and start a new thread, passing in the current object.

I've written an example of what I thought should do this, but it's not working. Specifically, it seems like the threads are getting overwritten on each iteration. This doesn't really make sense to me though because I'm making a new Thread object each time.

This is the test code I wrote

class Program
{
    static void Main(string[] args)
    {
        TestClass t = new TestClass();
        t.ThreadingMethod();
    }
}

class TestClass
{
    public void ThreadingMethod()
    {
        var myList = new List<MyClass> { new MyClass("test1"), new MyClass("test2") };

        foreach(MyClass myObj in myList)
        {
            Thread myThread = new Thread(() => this.MyMethod(myObj));
            myThread.Start();
        }
    }

    public void MyMethod(MyClass myObj) { Console.WriteLine(myObj.prop1); }
}

class MyClass
{
    public string prop1 { get; set; }

    public MyClass(string input) { this.prop1 = input; }
}

The output on my machine is

test2
test2

but I expected it to be

test1
test2

I tried changing the thread lines to

ThreadPool.QueueUserWorkItem(x => this.MyMethod(myObj));

but none of the threads started.

I think I just have a misunderstanding about how threads are supposed to work. Can someone point me in the right direction and tell me what I'm doing wrong?

Your life will be so much easier if you check out the Parallel Extensions Library introduced in .Net 3.5. Here's one place to start: http://msdn.microsoft.com/en-us/library/dd460693%28VS.100%29.aspx — DOK, Feb 23 '12 at 18:04

score 49 · Accepted Answer · answered Feb 23 '12 at 18:01

49

This is because you're closing over a variable in the wrong scope. The solution here is to use a temporary in your foreach loop:

    foreach(MyClass myObj in myList)
    {
        MyClass tmp = myObj; // Make temporary
        Thread myThread = new Thread(() => this.MyMethod(tmp));
        myThread.Start();
    }

For details, I recommend reading Eric Lippert's post on this exact subject: Closing over the loop variable considered harmful

answered Feb 23 '12 at 18:01

Reed Copsey

554,122
78
1,158
1,373

2

That was a quick response. Wow. – BlueM Feb 23 '12 at 18:06
1

Gah, I'm kicking myself because I actually read [this question](http://stackoverflow.com/q/8898925/817630) last month which is the answer. I even had a look at Eric's blog post. Thank you for pointing out what I should have remembered myself. – Kris Harper Feb 23 '12 at 18:18
4

@root45: Don't kick yourself too hard. This bites everyone, myself included. I've written that same bug a number of times over the years. – Eric Lippert Feb 23 '12 at 18:24
@EricLippert: I'd be willing to bet this is the most asked question on SO (disguised in various different ways of course). I am not kidding around when I say I have seen this problem in two other questions posted today alone. Any updates on making this a compiler warning (or some other solution) on the next version of C#? I know you have talked about it before. – Brian Gideon Feb 23 '12 at 18:42
1

@BrianGideon: Read my answer to this question: http://stackoverflow.com/questions/8898925/is-there-a-reason-for-cs-reuse-of-the-variable-in-a-foreach – Eric Lippert Feb 23 '12 at 18:43
@EricLippert: Ah yes, very nice! – Brian Gideon Feb 23 '12 at 18:46
this is a very bad implementation for threading especially if you have like 1000 items... – Desolator Feb 23 '12 at 19:09
@RobinVanPersi True - though the OP had 2 items. That being said, this directly addresses the issue, and helps the OP understand the problem (which isn't directly due to the threading implementation). I personally prefer using the TPL, but that would be an unrelated issue to the specific question at hand here. – Reed Copsey Feb 23 '12 at 19:16
@RobinVanPersi I agree, but I will only ever have 2 items (maybe 3). In the future I would like to update this with TPL, but this is an interim solution. – Kris Harper Feb 26 '12 at 01:07

score 4 · Answer 2 · answered Feb 23 '12 at 18:04

The problem is that you are using the most current value of the object inside of your closure. So, each invocation of the thread is looking at the same value. To get around this, copy the value into a local variable:

foreach(MyClass myObj in myList)
{
    MyClass localCopy = myObj;
    Thread myThread = new Thread(() => this.MyMethod(localCopy));
    myThread.Start();
}

score 2 · Answer 3 · answered Feb 23 '12 at 18:04

2

Agree with Reed's answer (+1).

I would add that if you are on .NET 4, you may want to look at the Task Parallel Library to solve this class of problem. Specifically for this case, have a look at Parallel.ForEach().

answered Feb 23 '12 at 18:04

Eric J.

147,927
63
340
553

1

While I like Parallel.ForEach - realize that it, itself, is a blocking method, where the OP's is "fire and forget" - so there is a functional difference using it. – Reed Copsey Feb 23 '12 at 18:09

score 1 · Answer 4 · answered Feb 23 '12 at 18:05

1

if sequence is not matter than go for

Parallel.ForEach(myList, obj => this.MyMethod(obj) );

Write a Simple Parallel.ForEach Loop

answered Feb 23 '12 at 18:05

Pranay Rana

175,020
35
237
263

Parallel.ForEach itself, is a blocking method, where the OP's is "fire and forget" - so there is a functional difference using it. This may not be appropriate in this case (at least not without putting it within a Task itself) – Reed Copsey Feb 23 '12 at 18:14

score 1 · Answer 5 · answered Feb 23 '12 at 18:09

1

I prefer this way:

public void ThreadingMethod()
{
    var myList = new List<MyClass> { new MyClass("test1"), new MyClass("test2") };


Parallel.ForEach(myList, new ParallelOptions() { MaxDegreeOfParallelism = 100 },
         (myObj, i, j) =>
         {
             MyMethod(myObj);
         });

}

not tested though....

answered Feb 23 '12 at 18:09

Desolator

22,411
20
73
96

Parallel.ForEach has a different functional behavior than the OP's method. The original code has "fire and forget" behavior, while this will block the calling thread until the operations complete. Also, I would not recommend setting MaxDegreeOfParallelism unless there is a specific reason to do so. – Reed Copsey Feb 23 '12 at 19:17
@ReedCopsey - I guess you are just assuming that. using Parallel.ForEach is always a preferred method to implement a PROPER threading in your application, especially if you have lots of items you want to process (at same time) and you don't have enough resources to use simple Thread(). its very poor implementation if he uses 1000 threads with your suggested method. – Desolator Feb 23 '12 at 19:52
Parallel.ForEach is not always a preferred method. It is a good option, but one of many. Using Tasks or the ThreadPool is typically a better option than managing the threading yourself, but even then, there are times when using Thread directly is still preferred. There isn't enough information in the OP's code to suggest that Parallel.ForEach would be a preferred option. (Granted, I'd almost always choose a long running Task over a manually managed Thread, though) – Reed Copsey Feb 23 '12 at 20:12

Starting a new thread in a foreach loop

5 Answers5

Linked

Related