CODEWITHSUNDEEP
javamultithreadingconcurrencyjava.util.concurrent
Ztyx
Ztyx

Reputation: 14929

Is it best-practise to Condition#signal eagerly?

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?

Upvotes: 2

Views: 69

Answers (2)

Jeffrey
Jeffrey

Reputation: 44808

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.

Upvotes: 1

cichystefan
cichystefan

Reputation: 328

Would my check have introduced a bug?

Yes. Condition takeptr == (items.length - 1) has nothing to do with filling the underlying buffer. It seems that the buffer is implemented in a "cycling" way.

In example code EVERY successful take makes buffer ready for a put, so you should signal every time. If not, threads will wait with "put", although there is free space in buffer...

Is it best-practise Java concurrent programming to eagerly signal when the condition is true?

As far as I remember, there is a general rule of thumb to signal after every event that may make condition change to true. But the proper condition checking is usually done by awaiting thread.

Your example scenario is quite simple (code that calls "signal" is sure that "notFull" condition will be met by any thread waiting on that "notFull" condition), but usually also signalAll is used instead of signal, as signal method signals only one thread (that may actually not meet the condition) and this may cause deadlocks.

Upvotes: 0

Related Questions