Race condition during thread start?

Question

Hi,

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?

Cheers!

Answer 1

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 2

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

Answer 3

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 4

Hello,

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

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 6

The problem seems to be there : () => WorkerThread(tmpItem, i)

I'm not used to Func<> but it seems to work like anonymous delegates in .NET 2.0. Thus, you may have a reference to the arguments of the WorkerThread() method. Hence, their values are retrieved later (when the thread actually runs).

In this case, you may already be at the next iteration of your main thread...

Try this instead :

var t = new Thread(new ParametrizedThreadStart(WorkerThread));
t.Start(new { ConfigurationItem = tmpItem, Index = i } );

[EDIT] Other implementation. More flexible if you need to pass new parameters to the thread in the future.

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

    foreach (ConfigurationItem tmpItem in config.getAllItems())
    {
            i++;
            var wt = new WorkerThread(tmpItem, i);
            wt.Start();
            //return t;
    }
}
private class WorkerThread
{
    private ConfigurationItem _cfgItem;
    private int _mul;
    private Thread _thread;
    public WorkerThread(ConfigurationItem cfgItem, int mul) {
        _cfgItem = cfgItem;
        _mul = mul;
    }
    public void Start()
    {
        _thread = new Thread(Run);
        _thread.Start();
    }
    private void Run()
    {
        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();
        });
    }
}

Answer 7

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 ;)