4

I have:

static public final ReentrantLock lock  = new ReentrantLock();
static public Condition my_condition    = lock.newCondition();

in myClass_1 and in myClass_2 class I call:

synchronized (myClass_1.my_condition){
    myClass_1.my_condition.signalAll();
}

This is giving me the java.lang.IllegalMonitorStateException. I am already synchronizing over the signall() call. What could be causing it?

VGR
  • 40,506
  • 4
  • 48
  • 63
Maths noob
  • 1,684
  • 20
  • 42

2 Answers2

8

This is because you are not getting the lock of ReentrantLock before signalling.

Read below important statements from ReentrantLock#newCondition

If this lock is not held when any of the Condition waiting or signalling methods are called, then an IllegalMonitorStateException is thrown.

Also, read below from Condition. Now, like you cannot call wait() if thread is not acquiring the lock, same you wait or signal conditions if lock is not acquired.

Where a Lock replaces the use of synchronized methods and statements, a Condition replaces the use of the Object monitor methods.


Bottom line: Acquire the lock before waiting or signalling the Condition.

lock.lock();  //Get the lock
while(/* whatever is your condition in myClass_1 and myClass_2 */){  //Or negative condition you want, but some code logic condition...
    my_condition.await();
}
my_condition_2.signal(); //If you want to notify one thread. Like in case of Java's blocking queue, if you want to notify one thread to put or take.
my_condition_2.signalAll(); //If you want to notify all threads.
hagrawal7777
  • 14,103
  • 5
  • 40
  • 70
  • I am achieving that ownership through `synchronized`. But I just changed signallAll to NotifyAll and I am no longer getting the error. – Maths noob Jul 22 '15 at 11:43
  • 1
    That worked because you were using `synchronized` keyword, which means the object's lock was held, so it was valid to call `notifyAll`. As per the code you provided, you were using `ReentrantLock` and `Condition` to achieve the conditional locking, and now it is of no use, because you have fallen back on Object's wait-notify mechanism. I have provided you the correct solution of problem which was arising because of `ReentrantLock` locking. If error in not coming doesn't necessarily mean that it is functioning as required, verify that you achieving the functional behavior you needed. – hagrawal7777 Jul 22 '15 at 11:53
  • I see. What is the mechanism that forces the syncrhonized(condition) to `Lock()/Synchronize` the `lock` as well? cause superficially it seems that the `Condition` works independently from the `lock`, besides the coupling that causes `IllegalMonitorStateException`. – Maths noob Jul 22 '15 at 12:22
  • Using `synchronized` means you want to work on Object's lock using `synchronized` methods and statements. `ReentrantLock` is like an extension of `synchronized` methods and statements which helps you to lock and unlock beyond one method (`synchronized` has method or block level scope and doesn't go beyond that to 2 or more methods). So, for locking either you use `synchronized` methods and statements OR `ReentrantLock`. In your case you were using `ReentrantLock` and a `Condition` deriving out it. – hagrawal7777 Jul 22 '15 at 12:35
  • This is one good online example I have searched for you, have a look - https://javabeanz.wordpress.com/2007/07/12/using-reentrant-locks-for-thread-synchronization/ – hagrawal7777 Jul 22 '15 at 12:41
  • in the documentation, we have: "Note that Condition instances are just normal objects and can themselves be used as the target in a synchronized statement, and can have their own monitor wait and notification methods invoked. Acquiring the monitor lock of a Condition instance, or using its monitor methods, has no specified relationship with acquiring the Lock associated with that Condition or the use of its waiting and signalling methods. It is recommended that to avoid confusion you never use Condition instances in this way, except perhaps within their own implementation." so why did it work? – Maths noob Jul 22 '15 at 12:45
  • as in I don't see how the synchronize over the `Condition` syncs over `Lock` too. I am trying to understand the coupling. – Maths noob Jul 22 '15 at 12:49
  • It worked because you were synchronizing over an object, what `synchronized` does is that it acquires the lock of the object, so as long as you are using some object with `synchronized` it is going to work. That's why I told earlier that verify you are achieving your functional requirement. I am not sure about your requirement or use case so I cannot comment on how to use, but the link I provided and also the example ReentrantLock documentation shows the correct way to use the `ReentrantLock` and `Condition` – hagrawal7777 Jul 22 '15 at 12:56
  • yeah sorry. I confused myself for a second. it only worked when I swtiched to NotifyAll() ie an object function. it's the `Condition`'s `SingallAll()` that complains about lock not being owned. So yeah like you said, I wasn't actually solving the potential problems that would have happened because of me not satisfying the lock requirement. – Maths noob Jul 22 '15 at 13:01
  • Not a problem buddy, make sure you are either using `synchronized` methods and statements OR `ReentrantLock` and `Condition`, as long as you really don't need both but for different purposes. – hagrawal7777 Jul 22 '15 at 13:05
3

Do not use synchronized with Locks. Locks and Conditions replace synchronized/wait/notify; they should never be used in combination with it.

The documentation for ReeantrantLock.newCondition states:

If this lock is not held when any of the Condition waiting or signalling methods are called, then an IllegalMonitorStateException is thrown.

Correct use of a Lock and Condition looks like this:

lock.lock();
try {
    someFlag = true;
    condition.signalAll();
} finally {
    lock.unlock();
}

And elsewhere:

lock.lock();
try {
    someFlag = false;
    while (!someFlag) {
        condition.await();
    }
} finally {
    lock.unlock();
}

All Condition.await* methods must be called in a while-loop that checks the data the Condition represents, since the await* methods are subject to spurious wakeups (just like the Object.wait* methods).

VGR
  • 40,506
  • 4
  • 48
  • 63
  • I see. It makes sense. By the way, will it be nessecary to use `try{} finally{}` for `signallAll()` since in my understanding, it is not going to throw any excepttion. – Maths noob Jul 22 '15 at 12:35
  • It is considered very good practice to place the call to unlock() in a finally block. It's probably better than taking a chance that a lock will remain forever held. – VGR Jul 22 '15 at 16:12
  • I understand, but isn't that practice in place, because of the existence of routines in the `try` block that throw exceptions? – Maths noob Jul 23 '15 at 08:28
  • You want to gurantee that it gets called no matter what, and the finally block is the way to do that. If you're asking whether try-catch-finally is as good as try-finally, the answer is yes, it's just as good. – VGR Jul 23 '15 at 12:52
  • thanks. What I mean is, what could possibly stop the try block and not the finally block if there is no exception. If it's an external thing that kills the thread hosting the try-finally blocks, surely it would stop the execution of the finally block too? – Maths noob Jul 23 '15 at 15:29
  • If I understand you correctly, the finally block is used because it is never safe to assume "if there is no exception." Every developer makes mistakes. A simple example would be if a later code change resulted in `condition` never being initialized, leaving it null. – VGR Jul 23 '15 at 16:10