Implementing a blocking queue in C#

Question

I've use the below code to implement and test a blocking queue. I test the queue by starting up 5 concurrent threads (the removers) to pull items off the queue, blocking if the queue is empty and 1 concurrent thread (the adder) to add items to the queue intermitently. However, if I leave it running for long enough I get an exception because one of the remover threads comes out of a waiting state even when the queue is empty.

Does anyone know why I get the exception? Note, I'm interested in knowing why this doesn't work as opposed to a working solution (as I can just Google that).

I'd greatly appreciate your help.

using System;
using System.Threading;
using System.Collections.Generic;

namespace Code
{
    class Queue<T>
    {
        private List<T> q = new List<T>();

        public void Add(T item)
        {
            lock (q)
            {
                q.Add(item);
                if (q.Count == 1)
                {
                    Monitor.Pulse(q);
                }
            }
        }

        public T Remove()
        {
            lock (q)
            {
                if (q.Count == 0)
                {
                    Monitor.Wait(q);
                }
                T item = q[q.Count - 1];
                q.RemoveAt(q.Count - 1);
                return item;
            }
        }
    }

    class Program
    {
        static Random r = new Random();
        static Queue<int> q = new Queue<int>();
        static int count = 1;
        static void Adder()
        {
            while (true)
            {
                Thread.Sleep(1000 * ((r.Next() % 5) + 1));
                Console.WriteLine("Will try to add");
                q.Add(count++);
            }
        }

        static void Remover()
        {
            while (true)
            {
                Thread.Sleep(1000 * ((r.Next() % 5) + 1));
                Console.WriteLine("Will try to remove");
                int item = q.Remove();
                Console.WriteLine("Removed " + item);
            }
        }

        static void Main(string[] args)
        {
            Console.WriteLine("Test");

            for (int i = 0; i < 5; i++)
            {
                Thread remover = new Thread(Remover);
                remover.Start();
            }

            Thread adder = new Thread(Adder);
            adder.Start();
        }
    }
}

Answer 1

if I leave it running for long enough I get an exception because one of the remover threads comes out of a waiting state even when the queue is empty. Does anyone know why I get the exception?

The question is odd, because obviously you know the answer: your first sentence answers the question asked by the second sentence. You get the exception because a remover thread comes out of the wait state when the queue is empty.

To solve the problem you'll want to use a loop instead of an "if". The correct code is:

while(q.Count == 0) Monitor.Wait(q);

not

if(q.Count == 0) Monitor.Wait(q);

UPDATE:

A commenter points out that perhaps your question was intended to be "under what circumstances can a consumer thread obtain the monitor when the queue is empty?"

Well, you are in a better position to answer that than we are, since you're the one running the program and looking at the output. But just off the top of my head, here's a way that could happen:

• Consumer Thread 1: waiting
• Consumer Thread 2: ready
• Producer Thread 3: owns the monitor
• There is one element in the queue.
• Thread 3 pulses.
• Thread 1 goes to ready state.
• Thread 3 abandons the monitor.
• Thread 2 enters the monitor.
• Thread 2 consumes the item in the queue
• Thread 2 abandons the monitor.
• Thread 1 enters the monitor.

And now thread 1 is in the monitor with an empty queue.

Generally speaking when reasoning about these sorts of problems you should think of "Pulse" as being like a pigeon with a note attached to it. Once released it has no connection to the sender, and if it cannot find its home, it dies in the wilderness with its message undelivered. All you know when you Pulse is that if there is any thread waiting then one thread will move to the ready state at some time in the future; you don't know anything else about the relative timing of operations on threads.

Answer 2

Your code would work if there was 1 consumer but when there are more, this mechanism fails and it should be while(q.Count == 0) Monitor.Wait(q)

The following scenario shows when if(q.Count == 0) Monitor.Wait(q) would fail (it's different than Eric's):

• consumer 1 is waiting
• producer has put in an item and is pulsing
• consumer 1 is ready
• producer is releasing lock
• consumer 2 just entered Remove, is lucky and acquires lock
• consumer 2 sees 1 item, does not wait and takes item out
• consumer 2 releases lock
• consumer 1 re-acquires lock but queue is empty

This happens exactly as documentation says it can happen:

When the thread that invoked Pulse releases the lock, the next thread in the ready queue (which is not necessarily the thread that was pulsed) acquires the lock.

Answer 3

Eric is of course right; the fact is that while the code appears to cover all the bases; the fact that an exception occurs shows that you haven't.

The race condition is that between the Monitor.Wait on a remover and a Monitor.Pulse on the adder (which releases the lock; but doesn't necessarily immediately trigger a thread waiting to wake up and reacquire it); a subsequent remove thread can acquire the lock and immediately jump the

if (q.Count == 0) 
{ 
  Monitor.Wait(q); 
} 

Statement and go straight to removing the item. Then, the Pulsed thread wakes up and assumes there's an item still there; but there isn't.

The way to fix it, whatever the way the race condition is actually manifesting, is as Eric has said.

Equally if you read the example on Monitor.Pulse you'll see a similar setup to what you have done here but a subtlely different way of doing it.