C# Multithreading duplicated

Question

I'm trying to make a tool that get source string from many URL I provided. And I use this code for multithreading

new Thread(() =>
{
    while (stop != true)
    {
        if (nowworker >= threads)
        {
            Thread.Sleep(50);
        }
        else
        {
            if (i <= urllist.Count - 1)
            {
                var thread = new Thread(() =>
                {
                     string source = GetSource(urllist[i]);
                     SaveToFile(source, i + ".txt"); 
                });
                thread.Start();
                i++;
                nowworker += 1;
            }
            else
            {
                stop = true;
            }

        }
    }
}).Start();

It's run very smooth until I check the result and have some duplicated result and missing some url I provided if using less thread for many url(10 thread - 20 url) but there's no problem when using 20 thread for 20 url.

Please help me. Thank you.

Answer 1

if (i <= urllist.Count - 1)
{
    var thread = new Thread(() =>
    {
         string source = GetSource(urllist[i]);
         SaveToFile(source, i + ".txt"); 
    });
    thread.Start();
    i++;
    nowworker += 1;
}

The method you're passing to the thread is not guaranteed to execute before i is updated (the i++). Infact, it's very unlikely that it will. This means that multiple threads may use the same value of i, and some values of i will not have any threads executing it.

Even worse, GetSource may use a different value of i than SaveToFile.

Have a readup here: http://jonskeet.uk/csharp/csharp2/delegates.html

This will fix it:

if (i <= urllist.Count - 1)
{
    var currentIndex = i;
    var thread = new Thread(() =>
    {
         string source = GetSource(urllist[currentIndex]);
         SaveToFile(source, currentIndex + ".txt"); 
    });
    thread.Start();
    i++;
    nowworker += 1;
}

Even better, you can replace the entire block of code with this:

Parallel.For(0, urlList.Count - 1, 
    new ParallelOptions { MaxDegreeOfParallelism = threads }, 
    i =>
    {       
        string source = GetSource(urllist[i]);
        SaveToFile(source, i + ".txt");
    }
);

Which will get rid of the code-smelly Thread.Sleep() and let .NET manage spinning up threads for you