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

Are you getting an exception somewhere? If so, your Interlocked.Decrement call won't happen for that element... — Reed Copsey, Jul 06 '12 at 15:58

score 3 · Accepted Answer · answered Jul 06 '12 at 16:00

3

There are a few issues in the above code:

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.
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.
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.
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.

answered Jul 06 '12 at 16:00

Reed Copsey

554,122
78
1,158
1,373

Could i use `lock(this)` instead? – dtsg Jul 06 '12 at 16:04
@Duane Why are you using locking at all? – Reed Copsey Jul 06 '12 at 16:04
I'm using the `result` to update a gridview on the UI thread within my `ReportProgress` method e.g. `outputGrid.Rows.Add(result.URL, result.Shares, result.Tweets, result.PlusOnes);` each time `ReportProgress` is called, i pass in the processed result and add a new row to the gv – dtsg Jul 06 '12 at 16:08
I'm using locking to prevent threads from accessing code that others are using, without it i was getting the same URL's processed multiple times – dtsg Jul 06 '12 at 16:13
@Duane In that case, there's absolutely no reason to use multiple threads. You're just adding overhead that provides no benefit. Just use one thread pool thread, and run the URLs in a loop... – Reed Copsey Jul 06 '12 at 17:10
But that's not what i want. I was previously using a BackgroundWorker and letting that process the URL's one by one while the UI updates itself, this works fine but was slow. What i'd like to do is fire off a bunch of threads from the ThreadPool allowing them to process more URL's at the same time, if that makes sense? – dtsg Jul 06 '12 at 18:09
@Duane You can't have a lock and process at the same time. The lock *specifically prevents that*, as it only allows one thread at a time through that section. You need to make your other items thread safe, so you can take out the lock. – Reed Copsey Jul 06 '12 at 18:41
Thanks for the help @Reed Copsey, i managed to solve the problem i was having but have another quick question if you can spare a moment. Regarding point #3, do you think it would be a good idea/safer to store those `checked` bools in a `state` object and pass that in along with the URL's to my `DoWork` method rather than accessing their values directly? – dtsg Jul 10 '12 at 08:09
@Duane Likely yes. Typically its best to pull your info out first, do your op, then set the UI at the end (or during progress reports) – Reed Copsey Jul 10 '12 at 15:29

score 0 · Answer 2 · answered Jul 10 '12 at 08:06

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

Correct way of checking when ThreadPool threads are done?

2 Answers2

Linked