My Produce Consumer Hangs

Question

Please copy the program below and try running in your IDE. It's a simple Produce Consumer implementation - it runs fine when I use one Producer and one Consumer thread but fails when using 2 each. Please let me know the reason why this program hangs or is there anything else wrong with it.

import java.util.LinkedList;
import java.util.Queue;

public class PCQueue {

 private volatile Queue<Product> productQueue = new LinkedList<Product>();

 public static void main(String[] args) {
  PCQueue pc = new PCQueue();

  Producer producer = new Producer(pc.productQueue);
  Consumer consumer = new Consumer(pc.productQueue);

  new Thread(producer, "Producer Thread 1").start();
  new Thread(consumer, "Consumer Thread 1").start();

  new Thread(producer, "Producer Thread 2").start();
  new Thread(consumer, "Consumer Thread 2").start();
 }

}

class Producer implements Runnable {

 private Queue<Product> queue = null;

 private static volatile int refSerialNumber = 0;

 public Producer(Queue<Product> queue) {
  this.queue = queue;
 }

 @Override
 public void run() {

  while (true) {
   synchronized (queue) {
    while (queue.peek() != null) {
     try {
      queue.wait();
     } catch (InterruptedException e) {
      // TODO Auto-generated catch block
      e.printStackTrace();
     }
    }
    queue.add(new Product(++refSerialNumber));
    System.out.println("Produced by: "
      + Thread.currentThread().getName() + " Serial Number: "
      + refSerialNumber);

    queue.notify();
   }
  }

 }
}

class Consumer implements Runnable {

 private Queue<Product> queue = null;

 public Consumer(Queue<Product> queue) {
  this.queue = queue;
 }

 @Override
 public void run() {
  while (true) {
   synchronized (queue) {
    while (queue.peek() == null) {
     try {
      queue.wait();
     } catch (InterruptedException e) {
      // TODO Auto-generated catch block
      e.printStackTrace();
     }
    }

    Product product = queue.remove();
    System.out.println("Consumed by: "
      + Thread.currentThread().getName() + " Serial Number: "
      + product.getSerialNumber());

    queue.notify();

   }
  }

 }

}

class Product {
 private int serialNumber;

 public Product(int serialNumber) {
  this.serialNumber = serialNumber;
 }

 public int getSerialNumber() {
  return serialNumber;
 }
}

Answer 1

instead of queue.notify() use queue.notifyAll()

Answer 2

The problem is that you are using queue.notify() which will only wake up a single Thread waiting on the Queue. Imagine Producer 1 calls notify() and wakes up Producer 2. Producer 2 sees that there is something in the queue so he doesn't produce anything and simply goes back to the wait() call. Now both your Producers and Consumers are all waiting to be notified and nobody is left working to notify anyone.

To solve the problem in your code, use queue.notifyAll() to wake up every Thread blocked at a wait(). This will allow your consumers to run.

As a note, your implementation limits the queue to having at most one item in it. So you won't see any benefit from the second set of producers and consumers. For a better all around implementation, I suggest you look at BlockingQueue and use an implementation which can be bounded, for instance, the ArrayBlockingQueue. Instead of synchronizing and using wait/notify, simply use BlockingQueue.offer() and BlockingQueue.take().