IllegalMonitorStateException despite calling notif

Question

public class Alternate {
    static Boolean mutex = true;
    public static void main(String[] args) {
        Thread t1 = new Thread(new Odd(mutex));
        Thread t2 = new Thread(new Even(mutex));
        t1.start();
        t2.start();
    }
}

class Odd implements Runnable{
    Boolean mutex;

    Odd( Boolean mutex){
        this.mutex=mutex;   
    }

    @Override
    public void run() {
        try {
            synchronized(mutex){
                while(mutex){
                    mutex.wait();
                }
                System.out.println("odd");
                mutex=true;
                mutex.notifyAll();
                Thread.sleep(500);
            }
        }catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

class Even implements Runnable{
    Boolean mutex;

    Even( Boolean mutex){
        this.mutex=mutex;
    }

    @Override
    public void run() {
        try {
            synchronized(mutex){
                while(!mutex){
                    mutex.wait();
                }
                System.out.println("even");
                mutex=false;
                mutex.notifyAll();
                Thread.sleep(500);
            }
        }catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

The error is

java.lang.IllegalMonitorStateException
    at java.lang.Object.notifyAll(Native Method)
    at com.test.concurrency.Even.run(Alternate.java:55)
    at java.lang.Thread.run(Thread.java:722)

I am not able to figure out the reason for the error. I am calling notifyAll() from synchronised context and calling it from the correct object.

Answer 1

Corrected Code if anyone needs

import java.util.concurrent.atomic.AtomicInteger;


public class Alternate {
     static final AtomicInteger mutex = new AtomicInteger(0);
    public static void main(String[] args) {
        Thread t1 = new Thread(new Odd());
        Thread t2 = new Thread(new Even());
        t1.start();
        t2.start();
    }



static class Odd implements Runnable{
    @Override
    public void run() {
        try {
            for(int i=0;i<10;i++){
                synchronized(mutex){
                    while(mutex.get()==1){
                        mutex.wait();
                    }
                    System.out.println("odd");
                    mutex.compareAndSet(0, 1);
                    mutex.notifyAll();
                }
            }

    }catch (InterruptedException e) {
        e.printStackTrace();
    }
    }
}
static class Even implements Runnable{

    @Override
    public void run() {
        try {
            for(int i=0;i<10;i++){
                synchronized(mutex){
                    while(mutex.get()==0){
                        mutex.wait();
                    }
                    System.out.println("even");
                    mutex.compareAndSet(1, 0);
                    mutex.notifyAll();
                }
            }   

        }catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

}
}

Answer 2

You're changing the lock out from under your threads. Every time you set your boolean to something, that's a different object; the code

            mutex=true;
            mutex.notifyAll();

sets mutex to a different object from the one the thread synchronized on (so the thread hasn't acquired the monitor for it), then it calls notifyAll on the new object.

Use a single lock and don't change it.

Locking on Booleans, numeric wrappers, or Strings is too error-prone and should be avoided. Not only can you end up with the error you're seeing, but other unrelated parts of the application (maybe written by somebody else following the same practice) could be locking on the same object and causing mysterious problems. Booleans, number wrappers, and strings are available to everything in the JVM. It's better to use a lock that is constrained in scope so that nothing else in your application can acquire it.

Often it's best to use a dedicated lock, something you don't use for any other purpose. Overloading something with different uses can cause trouble too easily.