does Monitor.Wait Needs synchronization?

Question

I have developed a generic producer-consumer queue which pulses by Monitor in the following way:

the enqueue :

    public void EnqueueTask(T task)
    {
        _workerQueue.Enqueue(task);
        Monitor.Pulse(_locker);
    }

the dequeue:

private T Dequeue()
    {
        T dequeueItem;
        if (_workerQueue.Count > 0)
        {
               _workerQueue.TryDequeue(out dequeueItem);
            if(dequeueItem!=null)
                return dequeueItem;
        }
        while (_workerQueue.Count == 0)
            {
                Monitor.Wait(_locker);
        }
         _workerQueue.TryDequeue(out dequeueItem);
        return dequeueItem;
    }

the wait section produces the following SynchronizationLockException : "object synchronization method was called from an unsynchronized block of code" do i need to synch it? why ? Is it better to use ManualResetEvents or the Slim version of .NET 4.0?

Answer 1

Yes, the current thread needs to "own" the monitor in order to call either Wait or Pulse, as documented. (So you'll need to lock for Pulse as well.) I don't know the details for why it's required, but it's the same in Java. I've usually found I'd want to do that anyway though, to make the calling code clean.

Note that Wait releases the monitor itself, then waits for the Pulse, then reacquires the monitor before returning.

As for using ManualResetEvent or AutoResetEvent instead - you could, but personally I prefer using the Monitor methods unless I need some of the other features of wait handles (such as atomically waiting for any/all of multiple handles).

Answer 2

From the MSDN description of Monitor.Wait():

Releases the lock on an object and blocks the current thread until it reacquires the lock.

The 'releases the lock' part is the problem, the object isn't locked. You are treating the _locker object as though it is a WaitHandle. Doing your own locking design that's provably correct is a form of black magic that's best left to our medicine man, Jeffrey Richter and Joe Duffy. But I'll give this one a shot:

public class BlockingQueue<T> {
    private Queue<T> queue = new Queue<T>();

    public void Enqueue(T obj) {
        lock (queue) {
            queue.Enqueue(obj);
            Monitor.Pulse(queue);
        }
    }

    public T Dequeue() {
        T obj;
        lock (queue) {
            while (queue.Count == 0) {
                Monitor.Wait(queue);
            }
            obj = queue.Dequeue();
        }
        return obj;
    }
}

In most any practical producer/consumer scenario you will want to throttle the producer so it cannot fill the queue unbounded. Check Duffy's BoundedBuffer design for an example. If you can afford to move to .NET 4.0 then you definitely want to take advantage of its ConcurrentQueue class, it has lots more black magic with low-overhead locking and spin-waiting.

Answer 3

The proper way to view Monitor.Wait and Monitor.Pulse/PulseAll is not as providing a means of waiting, but rather (for Wait) as a means of letting the system know that the code is in a waiting loop which can't exit until something of interest changes, and (for Pulse/PulseAll) as a means of letting the system know that code has just changed something that might cause satisfy the exit condition some other thread's waiting loop. One should be able to replace all occurrences of Wait with Sleep(0) and still have code work correctly (even if much less efficiently, as a result of spending CPU time repeatedly testing conditions that haven't changed).

For this mechanism to work, it is necessary to avoid the possibility of the following sequence:

• The code in the wait loop tests the condition when it isn't satisfied.

• The code in another thread changes the condition so that it is satisfied.

• The code in that other thread pulses the lock (which nobody is yet waiting on).

• The code in the wait loop performs a Wait since its condition wasn't satisfied.

The Wait method requires that the waiting thread have a lock, since that's the only way it can be sure that the condition it's waiting upon won't change between the time it's tested and the time the code performs the Wait. The Pulse method requires a lock because that's the only way it can be sure that if another thread has "committed" itself to performing a Wait, the Pulse won't occur until after the other thread actually does so. Note that using Wait within a lock doesn't guarantee that it's being used correctly, but there's no way that using Wait outside a lock could possibly be correct.

The Wait/Pulse design actually works reasonably well if both sides cooperate. The biggest weaknesses of the design, IMHO, are (1) there's no mechanism for a thread to wait until any of a number of objects is pulsed; (2) even if one is "shutting down" an object such that all future wait loops should exit immediately (probably by checking an exit flag), the only way to ensure that any Wait to which a thread has committed itself will get a Pulse is to acquire the lock, possibly waiting indefinitely for it to become available.