Correct way of checking when ThreadPool threads are done?

Question

I'm looking for a way of checking when all threads in threadpool have finished their tasks. Currently I'm using a counter that decrements when a thread has finished it's job and when counter == 0 I'm invoking my WorkComplete method. This seems to work but when i get to the final 'job' it doesn't appear to process the result? Or at least the UI doesn't get it. Here is what i currently have:

Queuing work items + incrementing counter

foreach (string s in URLs)
{
       ThreadPool.QueueUserWorkItem(new WaitCallback(DoWork), s);
       Interlocked.Increment(ref counter);
}

Do Work:

public void DoWork(object sender)
{      
    lock (_threadLock)
    {
        try
        {
            string url = (string)sender;
            result.URL = url;
            if (chkFb.Checked)
            {
                 result.Shares = grabber.GetFacebookShares(url);
            }
            if (chkTwitt.Checked)
            {
                 result.Tweets = grabber.GetTweetCount(url);
            }
            if (chkPlusOne.Checked)
            {
                 result.PlusOnes = grabber.GetPlusOnes(url);
            }
            Interlocked.Decrement(ref counter);
            this.Invoke(new ThreadDone(ReportProgress), result);
        }
        catch (Exception exc)
        {
            MessageBox.Show(string.Format("Errror: {0}", exc.Message);
        }
        finally
        {
            if (counter == 0)
            {
                this.Invoke(new ThreadDone(ReportProgress), result);
                this.Invoke(new Complete(WorkComplete));
            }
        }
    }
}

But the amount of URL's processed is always one less than the total count, it's almost like the last thread isn't 'Reporting back' or something. Does anyone have any ideas?

Thankyou

Answer 1

There are a few issues in the above code:

1. You're including exception handling, but your call to Interlocked.Decrement isn't in the finally block. This means that an exception will prevent the jobCounter from getting reduced properly.
2. You're locking within your method, on every thread. This effectively makes it so that only one of these ThreadPool threads can execute at any point in time, since they're all locking on the same variable (_threadLock). If this is desired, there's no reason to use multiple thread pool threads - just have one thread process all of the items in a loop, since thats effectively what you're doing now.
3. It appears (the code isn't 100% clear, though) that you're accessing UI elements from the thread pool threads directly (ie: chkTwitt.Checked). This isn't reliable.
4. You have everything being set into a single result variable, which is shared across all of the threads. Its not clear how this would be used, in a practical sense.

Given that you're effectively just processing a collection of items (URLs), you may want to also consider using Parallel.ForEach for even PLINQ to process the items.

Answer 2

Well I solved the issue i was having with this.

ScrapeResult result = new ScrapeResult();
string url = (string)sender;
result.URL = url;

if (chkFb.Checked)
{
    result.Shares = grabber.GetFacebookShares(url);
}
if (chkTwitt.Checked)
{
    result.Tweets = grabber.GetTweetCount(url);
}
if (chkPlusOne.Checked)
{
    result.PlusOnes = grabber.GetPlusOnes(url);
}

Interlocked.Decrement(ref counter);
this.Invoke(new ThreadDone(ReportProgress), result);

Rather than reusing the same variable result in my DoWork method, I create a new one for each thread so that there's no possibility of a thread getting any old/processed data (since I create a new instance for each one). This also solved some issues I was having with results being shown on the UI more than once.

I was also able to remove the lock (which made no sense in the first place) and have learnt a bit more about multi threading through this experience :)