runs false if statement when creating new threads

I'm writing a program that will email Cat Facts to anybody on a mailing list (not spamming, this is more of a learning experience than anything else). I have 2 arrays, 1 for facts (no problems) and one for emails. The one for emails will always throw an exception (even when in a try/catch block) and all of my if statements are completely ignored when this runs.

public void Broadcast() {
    fact++;
    if (fact ==factCount) {
        fact = 0;
    }
    String[] mailArray = mailingList.ToArray();
    String[] factArray = factList.ToArray();
    int i = mailArray.Length-1;
    String sendFact = factArray[fact];
    while (i>=0) {
        String s = mailArray[i];
        var t = new Thread(() => Mail(mailArray[i], sendFact));
        t.IsBackground = true;
        t.Start();
        i--;
        if (i==-1) {
            break;
        }
    }

The problem is here:

var t = new Thread(() => Mail(mailArray[i], sendFact));

Even when 'i' is -1, the while statement still runs and the break gets ignored. Originally I ran 'i' in reverse, starting at 0 then counting to mailArray.Length-1. It still gave the same issue, ignoring the while and break when I had them set to run in reverse.

Is this a problem with creating new threads?

Edit:

I moved:

    var t = new Thread(() => Mail(mailArray[i], sendFact));
    t.IsBackground = true;
    t.Start();

To a new method and called that instead of the code above - that works perfectly fine.

Jon Skeet
people
quotationmark

Is this a problem with creating new threads?

Sort of. It's a problem with capturing i in a lambda expression, and then changing it. It means that when the thread executes your lambda expression, it takes the current value of i, not the value of i in the loop iteration that you created the thread in.

The simplest solution is to declare a new local variable inside the loop, copy i into that, and then use that variable in the lambda expression:

while (i >= 0)
{
    var copy = i;
    String s = mailArray[i];
    var t = new Thread(() => Mail(mailArray[copy], sendFact));
    t.IsBackground = true;
    t.Start();
    i--;
}

Or just use s, which you've already initialized outside the lambda expression:

while (i >= 0)
{
    String s = mailArray[i];
    var t = new Thread(() => Mail(s, sendFact));
    t.IsBackground = true;
    t.Start();
    i--;
}

Or, rather more readably, IMO, just use a foreach loop:

foreach (var item in mailArray)
{
    Thread t = new Thread(() => Mail(item, sendFact)) { IsBackground = true };
    t.Start();
}

This will have the same problem under C# 3 and 4, but under C# 5 the behaviour of capturing the iterator variable in a foreach loop was fixed.

(You should consider using Task instead of Thread, and potentially using something like Parallel.ForEach instead, btw.)

people

See more on this question at Stackoverflow