Use of notifyAll instead of notify in the BoundedSemaphore implementation

Question

Sample implementation of BoundedSemaphore found on Jenkov's website uses notify instead of notifyAll in the take and release methods. In a scenario with multiple consumers and producers, wouldn't this cause a producer to wake up another producer (similarly consumer thread may wake up another consumer thread) leading to missed signals? Does notifyAll make more sense or am I missing something?

public class BoundedSemaphore {
  private int signals = 0;
  private int bound   = 0;

  public BoundedSemaphore(int upperBound){
    this.bound = upperBound;
  }

  public synchronized void take() throws InterruptedException{
    while(this.signals == bound) wait();
    this.signals++;
    this.notify();
  }

  public synchronized void release() throws InterruptedException{
    while(this.signals == 0) wait();
    this.signals--;
    this.notify();
  }
}

Producer/Consumer pattern uses notifyAll to avoid the problem of one consumer waking up another consumer. I was expecting the sample code to use notifyAll instead of notify.

Answer 1

I agree. It looks like a bad example.

Your BoundedSemaphore is equivalent to a bounded blocking queue of empty, indistinguishable tokens. IMO, that makes it appropriate to speak of "producers" who release() tokens to the queue, and "consumers" who take() tokens from the queue.

The example would work with no problems in a single-producer, single-consumer application. Although notify() only wakes one waiting thread, that's OK because there is only one other thread in the program that could possibly be waiting.

It won't work in a multi-producer or multi-consumer application because of exactly what you said: A producer could wake another producer, or a consumer could wake another consumer. Using notifyAll would fix that, but there's a better way....

---

...Instead of using synchronized and the built-in monitor of your BoundedSemaphore class, You could use a ReentrantLock object to synchronize the threads.

One advantage of ReentrantLock is, you can get two separate Condition objects for the lock. One Condition object would be awaited only by producers and notified only by consumers, and the other would be awaited only by consumers, and notified only by producers. Then, you call a condition's signal method, which behaves similarly to Object.notify(), and it would wake no more than one waiting thread, but it would be guaranteed to always wake the right kind of thread.

public class BoundedSemaphore {
  private int signals = 0;
  private int bound   = 0;
  private final ReentrantLock lock = new ReentrantLock();
  private final Condition condition_p = lock.newCondition();
  private final Condition condition_c = lock.newCondition();

  public BoundedSemaphore(int upperBound){
    this.bound = upperBound;
  }

  public void take() throws InterruptedException{
    lock.lock();
    try {
      while(this.signals == bound) condition_c.await();
      this.signals++;
      condition_p.signal();
    }
    finally {
      lock.unlock();
    }
  }

  public void release() throws InterruptedException{
    lock.lock();
    try {
      while(this.signals == 0) condition_p.await();
      this.signals--;
      condition_c.signal();
    }
    finally {
      lock.unlock();
    }
  }
}

---

Note: You might could make this code cleaner if you some kind of an AutoCloseable wrapper to lock the lock, and you used a try-with-resources statement to unlock it. I'll leave that as an exercise for the reader.