Ran across this MSDN page while searching for something else today. A “Community Content” response by “mike_msdn” asked the following question:

If NewItemEvent fires twice or more while the consumer thread is busy (i.e. not wating on WaitHandle.WaitAny), then won’t the consumer thread only be called once, missing the other calls?

The short answer to this is “yes”. Like many other code samples in MSDN, this one is utter crap. Just riddled with bugs. Be careful you understand the code when using any examples you find on the Internet, even if the source is one that should be authoritative.

(An interesting aside: there are folks at Microsoft who understand this topic, yet we still get broken samples in the MSDN.)

Like the title of this blog says, I’m going to go out on a limb and say that EventWaitHandle should be considered harmful. Actually, this isn’t the first time I went out on that limb, as when I worked on Boost.Threads I constantly had to explain how Win32 synchronization event objects were dangerous little buggers that should be avoided like the plague. I wish the things didn’t exist, as 9 times out of 10 (or worse) when someone uses an EventWaitHandle, they shouldn’t have. It’s a rare scenario in which an EventWaitHandle can be safely used, and an even rarer scenario in which it’s the solution you should choose.

So, what’s wrong with EventWaitHandle? It doesn’t address the issue of synchronizing access to shared state. This means that if shared state is involved you have to use some other synchronization mechanism in conjunction with the EventWaitHandle. However, this introduces race conditions between the wait and the lock. This is a subtle race condition as well… one that will go undetected for years, and then fail miserably at the worst possible time. Don’t believe me? Do a Google search on how to implement a “condition variable” on Win32. A condition variable is a special synchronization concept that combines the “unlock-wait-lock” set of operations into a single atomic operation, avoiding the race conditions I’m talking about here. When you do the Google search, the first thing you should notice is how complicated many of the solutions are. That should be enough to convince you that EventWaitHandle is the wrong solution in these scenarios. If not, really start to dig into the search results and see how most of those implementations have been proven to be broken. Then look at the implementation in Boost.Threads. If you can understand that implementation, then you have enough knowledge to safely use an EventWaitHandle… but you’ll also know better than to do so ;).

Back to the question by “mike_msdn". If the sample code is broken, how would you implement a producer consumer? Wikipedia has an entry on this. You could decide to use a Semaphore solution, as the article shows. This requires two Semaphore objects and a lock. Unlike EventWaitHandle solutions, when coded correctly, there’s no race condition between the Wait on the Semaphore and the lock, because we take advantage of the semantics of the Semaphore count. The other solution, and the simplest solution, is to use a “monitor”. Remember that “condition variable” I talked about before? Well, a “monitor” basically marries a “condition variable” and a “mutex” into a single concept. The .NET runtime has supported this concept since the beginning, with the Monitor static class. Here’s partial code to “fix” the buggy MSDN code, using a monitor (you should be able to figure out the missing pieces of code… I don’t have the time right now to create a fully working sample).

public class SyncObject
{
    public bool exit;
}

public class Producer
{
    private readonly Queue<int> _queue;
    private readonly SyncObject _sync;
    public Producer(Queue<int> q, SyncObject sync)
    {
        _queue = q;
        _sync = sync;
    }
    public void ThreadRun()
    {
        int count = 0;
        Random r = new Random();
        lock (_sync)
        {
            while (true)
            {
                while (_queue.Count >= 20)
                { // This loop waits for the consumer
                    Monitor.Wait(_sync);
                }
                Monitor.Wait(_sync, 0); // Release the lock to allow exit flag to be set
                if (_sync.exit)
                {
                    break;
                }
                _queue.Enqueue(r.Next(0, 100));
                Monitor.Pulse(_sync);
                count++;
            }
        }
        Console.WriteLine("Producer thread: produced {0} items", count);
    }
}

public class Consumer
{
    private readonly Queue<int> _queue;
    private readonly SyncObject _sync;
    public Consumer(Queue<int> q, SyncObject sync)
    {
        _queue = q;
        _sync = sync;
    }
    public void ThreadRun()
    {
        int count = 0;
        lock (_sync)
        {
            while (true)
            {
                while (_queue.Count == 0 && !_sync.exit)
                {
                    Monitor.Wait(_sync);
                }
                if (_sync.exit)
                {
                    break;
                }
                _queue.Dequeue();
                Monitor.Pulse(_sync);
                count++;
            }
        }
        Console.WriteLine("Consumer thread: consumed {0} items", count);
    }
}

Like I said, the Monitor stuff has been in .NET from the very beginning. It’s sad that very little code makes use of it. It’s scary that a lot of code that doesn’t is instead relying on buggy constructs, often using EventWaitHandle objects.