Race condition during thread start?

Question

I'm running the following code to start my threads, but they don't start as intended. For some reason, some of the threads start with the same objects (and some don't even start). If I try to debug, they start just fine (extra delay added by me clicking F10 to step through the code).

These are the functions in my forms app:

private void startWorkerThreads()
{
    int numThreads = config.getAllItems().Count;
    int i = 0;

    foreach (ConfigurationItem tmpItem in config.getAllItems())
    {
        i++;
        var t = new Thread(() => WorkerThread(tmpItem, i));
        t.Start();
        //return t;
    }
}

private void WorkerThread(ConfigurationItem cfgItem, int mul) 
{
    for (int i = 0; i < 100; i++)
    {
        Thread.Sleep(10*mul);
    }
    this.Invoke((ThreadStart)delegate()
    {
        this.textBox1.Text += "Thread " + cfgItem.name + " Complete!\r\n";
        this.textBox1.SelectionStart = textBox1.Text.Length;
        this.textBox1.ScrollToCaret();
    });
}

Anyone able to help me out?

Answer 1

Thanks to all of you!

I just implemented the threadpool, and that worked like a charm - with the added bonus of not spawning too many threads at once.

I'll have a look at the other solutions, too, but this time around the threadpool will save me from having to manually check for bozos with too many configs ;)

Answer 2

You can't assume that the threads will run in the same order they were called, unless you force it, and cause a dependency between them.

So the real question is - what is your goal ?

Answer 3

You just run into the (be me called) lambda error.

You provide the ConfigurationItem from the foreach loop directly. This leads to the fact, that all your threads get the same item (the last one).

To get this to work you have to create a reference for each item and apply this to each thread:

foreach (ConfigurationItem tmpItem in config.getAllItems())
{
        i++;
        var currentI = i;
        var currentItem = tmpItem;
        var t = new Thread(() => WorkerThread(currentItem, currentI));
        t.Start();
        //return t;
}

And you should also consider using a ThreadPool.

• MSDN Description about how to use the ThreadPool
• Short summary of differences here on SO

Answer 4

I think that the error is somewhere else. Here are some hints to help you debug :

1. Give a name containing to each thread, and display the thread name instead of the config item name :

   this.textBox1.Text += "Thread " + Thread.Current.Name + " Complete!\r\n";

2. Display the content of config.getAllItems(), may be that some items has the same name (duplicated)

===========

Here are some additional information about multi threading with winforms:

1. dont create new Thread directly, use the ThreadPool instead :

   ThreadPool.QueueUserWorkItem(state => { WorkerThread(tmpItem, i); });

2. If you really want to creat your threads, use this.BeginInvoke instead of this.Invoke your worker thread will finish sooner => less concurrent thread => better global performance
3. don't call Thread.Sleep in a loop, just do a big sleep: Thread.Sleep(10*mul*100);

I hope that this will help you.

Answer 5

Starting a thread doesn't really start the thread. Instead it schedules it for execution. I.e. at some point it will get to run when it is scheduled. Scheduling threads is a complex topic and an implementation detail of the OS, so your code should not expect a certain scheduling.

You're also capturing variables in your lambda. Please see this post (there is a section on Captured Variables) for the problems associated with doing that.

Answer 6

Do you really need to spawn threads manually (which is a rather expensive task) ? You could try to switch to the ThreadPool instead.