Missing line


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

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

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

  3. Stephen,

    I am playing with the synchronization primitives of .NET and probably I am doing something very wrong that is causing your class to crash when I do no have the line I added, here is the code:

    using System;

    using System.Threading;

    using System.IO;

    using System.Diagnostics;

    using NetMatters;

    namespace ConsoleApplication1

    {

    /// <summary>

    /// Summary description for Class2.

    /// </summary>

    public class Class2

    {

    private static int idx = 0;

    private static int[] numbers = new int[400];

    private static object synchInc = new object(); //To syncronize incrementer threads

    private static object synchWriters = new object(); //To synchronize writer threads

    private static System.IO.StreamWriter sw = new StreamWriter("C:\results.txt");

    private static ManualResetEvent mre = new ManualResetEvent(false); // To indicate file writer threads that the counter is ready to be written to the file

    private static ManualResetEvent mre1 = new ManualResetEvent(false); // To indicate counter incrementer threads that the counter was already written to the file

    static void Main()

    {

    for(int i=0; i<400;i++)

    {

    numbers[i] = i+1;

    }

    using (ThreadPoolWait tpw = new ThreadPoolWait())

    {

    // Queue up a bunch of items and wait for them all

    for(int i=0; i<2; i++)

    {

    tpw.QueueUserWorkItem(new WaitCallback(PrintNumber));

    tpw.QueueUserWorkItem(new WaitCallback(WriteToFile));

    }

    //while(!tpw.WaitOne());

    tpw.WaitOne();

    }

    Thread.Sleep(60000);

    }

    static void PrintNumber(object state)

    {

    while(idx<numbers.Length)

    {

    //Critical section to syncronize incrementer threads

    lock(synchInc)

    {

    // This try catch is an ugly solution to the problem of the counter going out of

    // bounds without the incrementer threads knowing

    try

    {

    Console.WriteLine("PrintNumber " + numbers[idx]);

    mre1.Set(); //signal the event for file writer threads

    mre.WaitOne(); //wait for event signal from the file writer threads

    idx++;

    mre.Reset();

    }

    catch(IndexOutOfRangeException ex)

    {

    Console.WriteLine("Ignore end of loop");

    break;

    }

    }

    }

    //This is another ugly solution to the problem of disposing of the mre objects after signal them for all the

    //threads to be able to finish. If we do not signal the mres then some threads are kept hanging, if we

    //do not call close we leak handles and other resources.

    try

    {

    mre1.Set();

    mre.Set();

    mre1.Close();

    mre.Close();

    }

    catch

    {

    }

    }

    static void WriteToFile(object state)

    {

    while(idx<numbers.Length)

    {

    //Critical section to synchronize writer threads

    lock(synchWriters)

    {

    // This try catch is an ugly solution to the problem of the counter going out of

    // bounds without the file writer threads knowing

    try

    {

    Console.WriteLine("WriteToFile " + numbers[idx]);

    mre1.WaitOne(); // Wait for signal from incrementer thread that the counter is ready to be written to the file

    sw.WriteLine(Thread.CurrentThread.Name + " " + numbers[idx]);

    mre1.Reset();

    mre.Set(); // Signal the incrementer threads that the counter was already written.

    }

    catch(IndexOutOfRangeException ex)

    {

    Console.WriteLine("Ignore end of the loop");

    break;

    }

    }

    }

    sw.Close();

    //This is another ugly solution to the problem of disposing of the mre objects after signal them for all the

    //threads to be able to finish. If we do not signal the mres then some threads are kept hanging, if we

    //do not call close we leak handles and other resources.

    try

    {

    mre1.Set();

    mre.Set();

    mre1.Close();

    mre.Close();

    }

    catch

    {

    }

    }

    }

    }

    Thanks for your help

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

  5. The exception I am getting if I removed the line I added is the following:

    Unhandled Exception: System.NullReferenceException: Object reference not set to

    an instance of an object.

      at NetMatters.ThreadPoolWait.DoneWorkItem() in c:documents and settingsbkugr01my documentsvisual studio projectsconsoletestconsoleapplication1threadpoolwait.cs:line 80

      at NetMatters.ThreadPoolWait.HandleWorkItem(Object state) in c:documents and

    settingsbkugr01my documentsvisual studio projectsconsoletestconsoleapplication1threadpoolwait.cs:line 73