Pass each list item to a new thread one by one

Question

What I'm trying to do is very simple,
I scan a list of strings, then, I pass each string to a new thread for printing.

using System;
using System.Collections.Generic;
using System.Threading;

namespace MultithreadingSynchronization
{
    class Program
    {
        static void Main(string[] args)
        {
            List<string> stringList = new List<string> { "server1", "server2", "server3", "server4", "server5", "server6", "server7", "server8", "server9"};

            foreach (string server in stringList)
            {
                ThreadStart work = delegate { Threadjob(server); };
                new Thread(work).Start();
                //Thread.Sleep(10); // 10 ms delay of main thread
            }
        }

        public static void Threadjob(object server)
        {
            Console.WriteLine(server);
        }
    }
}

From some reason, there are threads that received wrong value, therefore, the output presents some duplicated strings, and also miss some strings.
I'm expecting for this output (the order isn't important):

server1
server2
server3
server4
server5
server6
server7
server8
server9

But, sometimes I get this:

server3
server2
server5
server5
server7
server4
server8
server9
server9

and sometimes I get this:

server2
server2
server4
server3
server6
server7
server7
server8
server9

etc.

indeed, if I put a delay after each thread creating, I get what I expect to get.

Any idea?

I have tried your code (vs2012 C# console app default settings, win8.0 64) - and I never had repeating numbers. - I did get them out of order as you would expect with multi-threading, but not repeats. — , Dec 10 '13 at 19:55
Wouldn't you want your ThreadJob method to be an instance method? It doesn't appear to be thread safe — Cam Bruce, Dec 10 '13 at 19:55
It's because of the loop variable. [Closing over the loop variable considered harmful](http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx). Change to a for loop or use a local foreach variable as oakio suggests. — Patrick, Dec 10 '13 at 19:57
@WarpWars.Net, I'm using VS2010 (on Win 7), could that be the difference? — elady, Dec 10 '13 at 20:05
@elady yes: this behaviour is fixed in c# 5 because it confused too many people — Marc Gravell, Dec 10 '13 at 20:29

score 3 · Accepted Answer · edited May 23 '17 at 10:33

3

You should make local copy of variable. Try this:

    foreach (string server in stringList)
    {
        string local = server;
        ThreadStart work = delegate { Threadjob(local); };
        new Thread(work).Start();
        //Thread.Sleep(10); // 10 ms delay of main thread
    }

More info here: Captured variable in a loop in C#

edited May 23 '17 at 10:33

Community

1
1

answered Dec 10 '13 at 19:54

oakio

1,868
1
14
21

If you find a question that is a duplicate you should just vote/flag to close it as a duplicate, rather than posting an answer. – Servy Dec 10 '13 at 20:00

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

2

new Thread(Threadjob).Start(server);

Done! However, it may be more advisable to use tasks rather than threads - or at least ThreadPool.

answered Dec 10 '13 at 20:04

Marc Gravell

1,026,079
266
2,566
2,900

score 1 · Answer 3 · answered Dec 11 '13 at 08:07

OK, I think I get it.
Froeach loop hold a pointer, and changes it on each iteration. Sometimes, the thread is created, but still not running. meanwhile, the loop changes the pointer at the following iteration, and when the thread starts its job, it get the current pointer's value (the subsequent string).

BTW, I found another solution here, how to pass parameters to main function of the thread, so I fixed my loop and it works properly now

foreach (string server in stringList)
{
    Thread thread1 = new Thread(new ParameterizedThreadStart(Threadjob));
    thread1.Start(server); 
}

Pass each list item to a new thread one by one

3 Answers3