i have two methods to add and remove elements from a circular buffer

the first implementation :

synchronized void add(byte b) throws InterruptedException {

  if(availableObjects == size) wait();
  buffer[(tail 1)%size] = b ;
  tail  ;
  availableObjects  ;
  notifyAll();
}

synchronized byte remove() throws InterruptedException {

  if(head==tail) wait();
  Byte element = buffer[head%size];
  head    ;
  availableObjects--;
  notifyAll();
  return element;

}

and the second implementation :

private final Object addLock= new Object ();
private final Object removeLock=new Object ();

void add (byte b) throws InterruptedException{
    synchronized (addLock){
        while (availaibleObjects.get () == size) addLock.wait();
        buffer [tail]= b;
        tail = [tail   1) % size;
        availaibleObjects.incrementAndGet();}

    synchronized (removeLock){ // why we added this block ? 
        removeLock.notifyAll();}
    }


 byte remove () throws InterruptedException{
    byte element;
    synchronized (removeLock){
        while (availaibleObjects.get () == 0) removeLock.wait() ;
         element = buffer[head] ;
         head=(head   1) % size;
         availaibleObjects.decrementAndGet();}

        synchronized (addLock){ // why we added this block ? 
            addLock.notifyAll();}
            return element;}

my question is why in the second implementation of the methods we added a second synchronized block ?

1. from the first implementation i get that two threads cannot add and remove at the same time .
2. from the second implementation two threads can add and remove at the same time but i don't understand why we added the blocks :

synchronized (removeLock){ // why we added this block ? 
        removeLock.notifyAll();}


 synchronized (addLock){ // why we added this block ? 
            addLock.notifyAll();}
            return element;}

CodePudding user response：

my question is why in the second implementation of the methods we added a second synchronized block ?

Whenever you call wait() or notify() on an object, you must be in a synchronized block for that particular object. So if you want to call addLock.notifyAll(); you must be inside a synchronized (addLock). The synchronized keyword provides locking and also memory synchronization.

WARNING: More importantly, you have a bug because you are writing to buffer while inside the addLock synchronized block but reading inside removeLock. A writer could be inside addLock, add the item, and increment the availableObjects counter while a reader is already inside of addLock. This would mean that the reader might not see the updates to buffer and might read an element as null. If you are mutating buffer and then reading from it later, both should be holding in a synchronized block on the same object – you might as well lock on buffer. With this change you can then downgrade availableObjects to be a normal integer.

addLock.notifyAll();

If you do this right, you should be able to call notify() instead of notifyAll() since each element added or removed from the buffer will release at most 1 waiting thread.

if(availableObjects == size) wait();

It is important to point out that your 2nd implementation is correct to change these from if to while statements. It needs to be tested in a while loop otherwise you will have race conditions and bugs with multiple publishers and subscribers – also spurious wakeups on some architectures. See my page on these race conditions.