Compartilhar via

Missing line

  • Artigo

I received my October 2004 issue of MSDN Magazine today... woo hoo! I of course flipped to .NET Matters to see how it looked, and I noticed that somewhere during the process of putting out the issue I managed to omit a line of code from one of my code figures... oops!  It'll be fixed when the content goes online in several weeks, but in the meantime...

In the first column of Figure 3 on page 133 (don't you love it when a magazine has over 130 pages... 144 this month!), there is a block of code that looks like:

    if (rv)
    {
        _remainingWorkItems = 1;
        _done.Reset();
    }

The line after that was omitted and should read:

    else Interlocked.Increment(ref _remainingWorkItems);

What's the benefit of that line?  It's not the focus of the column so I gloss over it, but without this line all is well unless WaitOne is called more than once before all work items have completed.  So if you use the overload of WaitOne that takes no parameters (and thus wait until all items have completed), everything should work correctly.  If, however, you use an overload that takes a timeout parameter, and that timeout expires before the work items have completed, you'll have a problem should you call WaitOne again.  The DoneWorkItem method called at the beginning of the function will have already decremented _remainingWorkItems in order to account for _remainingWorkItems initially being 1 (the reason for which is explained in the column).  The next time WaitOne is called, however, DoneWorkItem will be called again to remove that initial value of 1, but it was already removed, so _remainingWorkItems will be erroneously decremented.  This could result in _done being set too soon, causing ThreadPoolWait to erroneously show that all work items had completed.  Thus, the Interlocked.Increment line is necessary to bump the value back up so that the next time WaitOne is called, _remainingWorkItems will still be valid.  It's fine, too, if by the time this line is executed all of the work items have finished.  The next time WaitOne is called, the value of _remainingWorkItems will be 1 (as a result of the increment up from 0), and DoneWorkItem will remove that value and set _done; it doesn't matter that _done will have already been set when the last work item finished, since setting an already set ManualResetEvent is for all intents and purposes a noop.

Note that in the 'if' block above I don't have to use the Interlocked class to set the value of _remainingWorkItems to 1 because at that point rv == true, which means that all work items have completed and no other threads will be mucking with the value.

Comments

  • Anonymous
    September 15, 2004
    This article is now available at http://msdn.microsoft.com/msdnmag/issues/04/10/NetMatters.
  • Anonymous
    September 19, 2006
    Stephen,

    I am using your ThreadPoolWait class but I had to apply the following change:

    private void DoneWorkItem()
    {
    if ((Interlocked.Decrement(ref _remainingWorkItems) == 0) && (_done != null))
    _done.Set();
    }
    If I remove the _done != null in some scenarios I get an exception.

    Thanks,

    Gilbert
  • Anonymous
    September 20, 2006
    Gilbert, that implies to me that you're using the class in a way it wasn't intended to be used... that change shouldn't be necessary.  Can you send me a repro?
  • Anonymous
    September 25, 2006
    The comment has been removed
  • Anonymous
    September 25, 2006
    What is the exception you're getting?  The only exception I get when I run your code is an error from calling mre.WaitOne and mre1.WaitOne, because depending on which of the queued callbacks executes and completes first, you may have already Close'd those reset events.
  • Anonymous
    September 26, 2006
    The comment has been removed