Producer/Consumer in C#

Question

Is this code an adequate implementation of the producer/consumer pattern?

In computer science, the producer/consumer pattern is a classic example of multithreaded synchronisation. The problem describes some threads called consumers and some threads called producers sharing a common fixed-size queue.

In the above code, the queue is called _myQueue and it's size is 1000.

Each producer job is to generate some item, put it in the queue by calling AddItem(), and start again. The consumers job is to remove items from the queue by calling RemoveItem()

The problem is to make sure no producer tries to add items to the queue when it's full (i.e. containing 1000 items) and no consumer tries to remove items from the queue when it's empty.

Also, we'd like to avoid situation where consumers or producers end up waiting for no reason (deadlocks).

private int _count = 1000;
private Queue<string> _myQueue = new Queue<string>();
private static object _door = new object();

public void AddItem(string someItem)
{
    lock (_door)
    {
        while (_myQueue.Count == _count)
        {
            Monitor.Wait(_door);
        }

        _myQueue.Enqueue(someItem);
        Monitor.Pulse(_door);
    }
}

public string RemoveItem()
{
    string item = null;
    lock (_door)
    {
        while (_myQueue.Count == 0)
        {
            Monitor.Wait(_door);
        }

        item = _myQueue.Dequeue();
        Monitor.Pulse(_door);
     }

    return item;
}

Answer 1

Answering whether or not this implementation is correct would require to know what the specification of the problem is. Now assuming the spec is:

Create a queue to which multiple threads can add items and multiple threads can remove item. The queue will hold no more than 1000 string references and blocks writers when the queue is full and readers when the queue is empty.

then I'd say yes your implementation will achieve that.

Some remarks:

1. As mentioned by @svick you should seriously consider using BlockingCollection which was added in .NET 4.0.
2. You can increase the practical usefulness immediately by doing two simple things
   1. Make your class generic. This way you can create a blocking queue for any type not just string.
   2. Let the user pass in the capacity.

3. Some future speculation in case you are stuck with .NET 3.5 or earlier and want to stick with your implementation: If you are ever tempted to implement IEnumerable<T> (to show the content in a UI for example) on your class then remember that the following implementation is broken (I have seen this more often than I'd like):

   public IEnumerator<T> GetEnumerator()
   {
       lock (_door)
       {
           return _myQueue.GetEnumerator();
       }
   }
   

   The correct way would be to make a local copy of the queue (inside the lock) and return the enumerator to that. The other option is to iterate over the queue inside the lock and yield the items which effectively locks the queue for the entire duration of the enumeration. However this is probably a bad idea since a caller which is not aware of this can easily lock out all access to the queue for a long period of time.