I implemented a barrier in java using synchronized block and got IllegalMonitorStateException. On replacing that synchronized block with explicit locking with lock.lock() and lock.unlock() in await method, the code works as expected.
I could not figure out what exactly is happening different between the cases. Can someone help me with understanding this?
package org.example;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
public class Barrier {
private int count;
private int threshold;
private final Lock lock;
int toBeReleased;
Condition incrementCondition;
Condition releaseCondition;
public Barrier(int threshold) {
this.count = 0;
this.toBeReleased = 0;
this.threshold = threshold;
this.lock = new ReentrantLock();
this.incrementCondition = lock.newCondition();
this.releaseCondition = lock.newCondition();
}
public void await () throws InterruptedException {
- synchronized(lock) {
+ lock.lock();
while (count == threshold) {
incrementCondition.await();
}
++count;
if (count == threshold) {
toBeReleased = threshold;
releaseCondition.signalAll();
} else {
// Note the usage of await/signal and not wait/notify
releaseCondition.await();
}
toBeReleased--;
if (toBeReleased == 0) {
count = 0;
incrementCondition.signalAll();
}
+ lock.unlock();
- }
}
public static void main(String[] args) throws InterruptedException {
final Barrier barrier = new Barrier(3);
Thread p1 = new Thread(new Runnable() {
public void run() {
try {
System.out.println("Thread 1");
barrier.await();
System.out.println("Thread 1");
barrier.await();
System.out.println("Thread 1");
barrier.await();
} catch (InterruptedException ignored) {}
}
});
Thread p2 = new Thread(new Runnable() {
public void run() {
try {
Thread.sleep(500);
System.out.println("Thread 2");
barrier.await();
Thread.sleep(500);
System.out.println("Thread 2");
barrier.await();
Thread.sleep(500);
System.out.println("Thread 2");
barrier.await();
} catch (InterruptedException ignored) {}
}
});
Thread p3 = new Thread(new Runnable() {
public void run() {
try {
Thread.sleep(1500);
System.out.println("Thread 3");
barrier.await();
Thread.sleep(1500);
System.out.println("Thread 3");
barrier.await();
Thread.sleep(1500);
System.out.println("Thread 3");
barrier.await();
} catch (InterruptedException ie) {}
}
});
p1.start();
p2.start();
p3.start();
p1.join();
p2.join();
p3.join();
}
}
As a general rule, if you're using java.util.concurrent.locks, then do not use synchronized in the same context. The same is true in reverse. The two are separate mechanisms and are not designed to interact with each other.
All objects in Java have an implicit "monitor". When you synchronize on an object, you acquire that monitor. While you hold the monitor, you can call the following methods on the corresponding object instance:
Object#wait(), Object#wait(long), Object#wait(long,int)Object#notify()Object#notifyAll()Trying to call these methods on an object instance without holding that object's monitor will result in an IllegalMonitorStateException being thrown.
Unlike synchronized, this API is not built into the language. It does not care or know about the implicit monitor. Instead, a Lock maintains its own state regarding which thread owns it, and which threads are waiting to acquire it. To acquire and release a Lock, you must go through the methods defined by the Lock interface; you cannot use synchronized.
One of the things the java.util.concurrent.locks API did was extract the concept of a "condition" out into its own object. This makes Lock a "factory" for Condition objects, and a Condition remains associated with its source Lock. Thus, a Condition knows whether a thread calling one of its methods owns the corresponding Lock or not. And similar to Lock, waiting and signaling must be done via the methods of Condition; you must not use the methods defined by the Object class.
Trying to call methods on a Condition (or trying to unlock a Lock) without owning the Lock (via a call to e.g., Lock#lock()) will result in an IllegalThreadStateException being thrown.
Note this API also provides ReadWriteLock and StampedLock, as well as the building blocks needed to write your own lock implementations.
You essentially have the following:
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
public class Foo {
private final Lock lock = new ReentrantLock();
private final Condition condition = lock.newCondition();
public void bar() throws InterruptedException {
synchronized (lock) {
condition.await();
}
}
}
This is mixing the two locking mechanisms together, which will not work properly. You synchronized on the lock, thus acquiring the object's implicit monitor. But then you call await() on the Condition object. You may own the monitor, but from the point-of-view of the Lock and Condition, you do not own the lock. Hence an IllegalThreadStateException being thrown.
You should use either synchronized or java.util.concurrent.locks. The following is two versions of a "barrier" that waits for a certain number of threads to be waiting before letting them all continue on. One version uses synchronized, the other version uses java.util.concurrent.locks.
synchronized versionpublic class SynchronizedBarrier implements Barrier {
private final Object lock = new Object();
private final int threshold;
private int count;
private boolean thresholdReached;
public SynchronizedBarrier(int threshold) {
if (threshold <= 0) throw new IllegalArgumentException("threshold <= 0");
this.threshold = threshold;
}
@Override
public void await() throws InterruptedException {
synchronized (lock) {
if (thresholdReached) throw new IllegalStateException("threshold already reached");
count++;
if (count == threshold) {
thresholdReached = true;
lock.notifyAll();
} else {
while (!thresholdReached) {
try {
lock.wait();
} catch (InterruptedException ex) {
count--;
throw ex;
}
}
}
}
}
}
java.util.concurrent.locks versionimport java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
public class LocksBarrier implements Barrier {
private final Lock lock = new ReentrantLock();
private final Condition isThresholdReached = lock.newCondition();
private final int threshold;
private int count;
private boolean thresholdReached;
public LocksBarrier(int threshold) {
if (threshold <= 0) throw new IllegalArgumentException("threshold <= 0");
this.threshold = threshold;
}
@Override
public void await() throws InterruptedException {
lock.lockInterruptibly();
try {
if (thresholdReached) throw new IllegalStateException("threshold already reached");
count++;
if (count == threshold) {
thresholdReached = true;
isThresholdReached.signalAll();
} else {
while (!thresholdReached) {
try {
isThresholdReached.await();
} catch (InterruptedException ex) {
count--;
throw ex;
}
}
}
} finally {
lock.unlock();
}
}
}
Both versions are single-use. If the threshold is reached, then no more threads can call await() without an exception being thrown. I also implemented it so that a waiting thread that's interrupted is subtracted from the number of waiting threads. That choice was arbitrary. You could leave count unmodified. Or maybe you could have all other non-interrupted threads throw a BrokenBarrierException (similar to how CyclicBarrier does it).