Is it best-practise to Condition#signal eagerly?

Question

The Condition JavaDoc has the following code sample:

class BoundedBuffer {
   final Lock lock = new ReentrantLock();
   final Condition notFull  = lock.newCondition(); 
   final Condition notEmpty = lock.newCondition(); 

   final Object[] items = new Object[100];
   int putptr, takeptr, count;

   public void put(Object x) throws InterruptedException {
     lock.lock();
     try {
       while (count == items.length)
         notFull.await();
       items[putptr] = x;
       if (++putptr == items.length) putptr = 0;
       ++count;
       notEmpty.signal();
     } finally {
       lock.unlock();
     }
   }

   public Object take() throws InterruptedException {
     lock.lock();
     try {
       while (count == 0)
         notEmpty.await();
       Object x = items[takeptr];
       if (++takeptr == items.length) takeptr = 0;
       --count;
       notFull.signal();
       return x;
     } finally {
       lock.unlock();
     }
   }
 }

If I would have implemented BoundedBuffer#take, I would only have called notFull.signal() only when count == (items.length - 2) to avoid signalling other threads unless necessary. I also note that ArrayBlockingQueue#removeAt is eagerly calling notFull.signal();.

Question(s): Would my check have introduced a bug? Is it best-practise Java concurrent programming to eagerly signal when the condition is true? I assume it will reduce the risk of a deadlock. Does it have any performance implications?

Answer 1

Yes. Your check introduces a bug. It's fairly easy to show with a pathological example.

Let's assume count = items.length:

Thread1: put(o1); // count == items.length. Waiting on notFull.
Thread2: put(o2); // Waiting on notFull.
Thread3: put(o3); // Waiting on notFull.
Thread4: take();  // count -> items.length - 1
                  // There's space in the buffer, but we never signalled notFull.
                  // Thread1, Thread2, Thread3 will still be waiting to put.
         take();  // count -> items.length - 2, signal notFull!
                  // But what happens if Thread4 manages to keep the lock?
         take();  // count -> items.length - 3
         ...
         take();  // count -> 0
Thread1: // Wakes up from Thread4's signal.
         put(o1); // count -> 1
Thread2: put(o2); // Never signalled, still waiting on notFull.
Thread3: put(o3); // Never signalled, still waiting on notFull.

This could be mitigated using signalAll, but you'll still have a couple issues:

1. Since you're waking up every thread even when only one space was opened, there will be more lock contention.
2. You will still have threads waiting to put when you have exactly one space open in your buffer. Depending on the implementation/documentation I guess this could be okay, but it would still be rather surprising in most cases.