NotifyAll, IllegalMonitorStateException

Question

import java.math.BigInteger;

class Numbers {

    final static int NUMBER = 2;
    final static int POWER = 4;

    static long msecs;
    static BigInteger result;
    static Boolean done = false;

    public static void main(String[] args) {

        BigInteger number = BigInteger.valueOf(NUMBER);
        result = number;

        //Boolean done = false;

        Runnable pow = () -> {
            System.out.println(number + " pow " + POWER + " = " + number.pow(POWER));

            synchronized (done) {
                done = true;
                done.notifyAll();
            }
        };

        Runnable sum = () -> {
            for(int i = 2; i<POWER; i=i*i) {
                result = result.multiply(result);
            }

            System.out.println(number + " sum " + POWER + " = " + result);

            synchronized (done) {
                done = true;
                done.notifyAll();
            }
        };

        Runnable time = () -> {
            for(msecs = 0; true; msecs++) {
                try {
                    Thread.sleep(1);
                } catch(InterruptedException e) {
                    //nic
                }
            }
        };

        Thread timet = new Thread(time);
        Thread sumt = new Thread(sum);
        Thread powt = new Thread(pow);

        timet.start();
        powt.start();

        synchronized (done) {
            while(!done) {
                try {
                    done.wait();
                } catch (InterruptedException e) {
                    //nic
                }
            }
        }

        timet.interrupt();
        powt.interrupt();

        System.out.println("Pow time " + msecs + " msecs.");
        done = false;

        timet.start();
        sumt.start();

        try {
            synchronized (done) {
                while (!done) {
                    done.wait();
                }
            }
        } catch (InterruptedException e) {
            //nic
        }


        timet.interrupt();
        sumt.interrupt();

        System.out.println("Sum time " + msecs + " msecs.");

    }
}

I want to check time differences between those two methods, but done.notifyAll() keeps throwing IllegalMonitorStateException.

score 8 · Accepted Answer · edited May 23 '17 at 11:44

8

Problem is here:

synchronized (done) {
    done = true;//<---problem
    done.notifyAll();
}

Since you are assigning new value to done it means you are executing notifyAll on Boolean.TRUE but your synchronization block is using monitor of Boolean.FALSE. And since notifyAll requires thread to posses monitor of object on which it is executed it throws IllegalMonitorStateException.

So don't change value of object on which you synchronize. Also avoid synchronizing on objects which are available for all classes (public constants /literals) since you are risking that other people will have same idea and will also use them in their synchronization which could cause you some pain.

Locks should be accessible only from within class to they belong. So take example from Jon Skeet (What is the difference between synchronized on lockObject and using this as the lock?) and synchronize on your own

private final Object lock = new Object();

edited May 23 '17 at 11:44

Community

1
1

answered Jan 18 '16 at 23:19

Pshemo

122,468
25
185
269

Yup. Try doing what I did above, where I synchronized on a doneLock object instead. – ngoctranfire Jan 18 '16 at 23:24
I'm confused. Are you saying there are really only two distinct Boolean objects in the JVM? – President James K. Polk Jan 18 '16 at 23:25
2

@JamesKPolk We can create many Boolean objects via `new Boolean(true/false)`, but autoboxing is avoiding this and is reusing `Boolean.TRUE`/`FALSE` to save us space. Main thing to realize here is that `done` now contains different value than it was synchronized on. – Pshemo Jan 18 '16 at 23:29
1

It should be emphasized that it is a different *object instance* as the term “value” might be misleading. By the way, every IDE should scream at this code. But the worst offender is the `long msecs` updated and read by different threads without any synchronization. Yeah, and interrupting a thread which will simply catch the `InterruptedException` and proceed with its loop, won’t have much effect… – Holger Jan 19 '16 at 09:26

ngoctranfire · Answer 2 · 2016-01-19T00:36:38.313

I think I know what's happening.... If you read Java's documentation for Object.notifyAll()

IllegalArgumentException - if the value of timeout is negative. IllegalMonitorStateException - if the current thread is not the owner of the object's monitor. InterruptedException - if any thread interrupted the current thread before or while the current thread was waiting for a notification. The interrupted status of the current thread is cleared when this exception is thrown.

Therefore, I think you need to have a new Object that acts as a lock for accessing your done. For example: a private final Object doneLock = new Object(). Instead of synchronizing on Done, you would now synchronize on doneLock. I should be final because locks need to be private and final so they can't be accessed outside.

For example,

synchronize(doneLock) {
    done = true;
    doneLock.notifyAll();
}

NotifyAll, IllegalMonitorStateException

2 Answers2