5
\$\begingroup\$

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;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Why would you write your own instead of using BlockingCollection? Also, does it seem to work for you? \$\endgroup\$ – svick Oct 24 '13 at 0:20
  • \$\begingroup\$ @svick I'm aware of the existence of this class in .NET 4 but this is more of a personal challenge than a real world problem. My implementation seems to work but I wanted to make sure I didn't overlook something. \$\endgroup\$ – csharper Oct 24 '13 at 22:53
  • \$\begingroup\$ @svick : One reason to avoid using BlockingCollection, is simply if you can't use .NET 4 features, either because you are forced in a production environment without it, or because your development environment doesn't support that (e.g. Unity 3D). \$\endgroup\$ – Marco Mp Oct 24 '14 at 15:56
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Sorry if I wasn't clear enough. I edited my question. The spec you assumed is correct. I want to make sure as well my implementation doesn't result in a deadlock. This is more of a personal challenge than a real world problem, hence not using BlockingCollection or Generics. +1 for the GetEnumerator() tip! \$\endgroup\$ – csharper Oct 24 '13 at 23:04
  • 1
    \$\begingroup\$ Peeking in the source of BlockingCollection, it offers an both an enumerator which makes a copy and a consuming enumerable which modifies the collection as it is enumerated. \$\endgroup\$ – Brian Oct 25 '13 at 17:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.