Volatile keyword without static not working as expected

Question

I understood the difference between volatile and static keywords on variables.

static variables can be changed by different instances whereas Volatile variables can be changed by different threads.

But the below program(Copied from internet and little modified) hangs, if i remove the static keyword for MY_INT variable.

The update to variable MY_INT should be seen by other thread even without static key word. But if i remove static it hangs.

Please help me to understand this issue.

public class PrintOddAndEven extends Thread {
    static volatile  int i = 1;

    Object lock;

    PrintOddAndEven(Object lock) {
        this.lock = lock;
    }

    public static void main(String ar[]) {
        Object obj = new Object();
        PrintOddAndEven odd = new PrintOddAndEven(obj);
        PrintOddAndEven even = new PrintOddAndEven(obj);
        odd.setName("Odd");
        even.setName("Even");
        odd.start();
        even.start();
    }

    @Override
    public void run() {
        while (i <= 10) {
            if (i % 2 == 0 && Thread.currentThread().getName().equals("Even")) {
                synchronized (lock) {
                    System.out.println(Thread.currentThread().getName() + " - " + i);
                    i++;
                    lock.notify();
                }
            }
            if (i % 2 == 1 && Thread.currentThread().getName().equals("Odd")) {
                synchronized (lock) {
                    System.out.println(Thread.currentThread().getName() + " - " + i);
                    i++;

                    try {
                        lock.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }
}

Answer 1

Your bug is due to the fact that if you remove the static keyword from the field i, you will have one different field i per instance of PrintOddAndEven so here as you have 2 instances, you have 2 different fields i such that the Even thread will loop forever as its i is never modified and the Odd thread waits forever for the same reason. When you declare the field as static, the threads share the same field i such that you don't face your bug.

You should create a dedicated class that will hold your counter and use an instance of it as object monitor that you will share between the PrintOddAndEven instances, as next:

public class MyClass {
    volatile int i = 1;
}

public class PrintOddAndEven extends Thread {

    MyClass lock;

    PrintOddAndEven(MyClass lock) {
        this.lock = lock;
    }

    public static void main(String[] args) throws Exception {
        MyClass obj = new MyClass();
        PrintOddAndEven odd = new PrintOddAndEven(obj);
        PrintOddAndEven even = new PrintOddAndEven(obj);
        odd.setName("Odd");
        even.setName("Even");
        odd.start();
        even.start();
    }

    @Override
    public void run() {
        while (lock.i <= 10) {
            if (lock.i % 2 == 0 && Thread.currentThread().getName().equals("Even")) {
                synchronized (lock) {
                    System.out.println(Thread.currentThread().getName() + " - " + lock.i);
                    lock.i++;
                    lock.notify();
                }
            }
            if (lock.i % 2 == 1 && Thread.currentThread().getName().equals("Odd")) {
                synchronized (lock) {
                    System.out.println(Thread.currentThread().getName() + " - " + lock.i);
                    lock.i++;

                    try {
                        lock.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }
}

---

If you have only a counter, you could also consider using an instance of class AtomicInteger as counter and object monitor. The code will be the same as above except that you will create an instance using new AtomicInteger(1) to initialize the counter to 1 and then use get() to get the current value and incrementAndGet() to increment the counter.

Answer 2

you have created 2 PrintOddAndEven odd and even thread objects, if you remove Static keyword from this statement volatile int i = 1; i didnot remain class level and hence every thread object has its own copy of i, when odd thread execute it, it updated the odd.i++. However even.i remain 1, and the condition is not passed and it seams that your thread is hangs without static.

public class PrintOddAndEven extends Thread {
//static volatile  int i = 1;

Lock lock;

PrintOddAndEven(Lock lock) {
    this.lock = lock;
}


static class Lock {
     volatile  int i = 1;
}
public static void main(String ar[]) {
    Lock obj = new lock();
    PrintOddAndEven odd = new PrintOddAndEven(obj);
    PrintOddAndEven even = new PrintOddAndEven(obj);
    odd.setName("Odd");
    even.setName("Even");
    odd.start();
    even.start();
}

@Override
public void run() {
    while (lock.i <= 10) {
        if (lock.i % 2 == 0 && Thread.currentThread().getName().equals("Even")) {
            synchronized (lock) {
                System.out.println(Thread.currentThread().getName() + " - " + lock.i);
                lock.i++;
                lock.notify();
            }
        }
        if (lock.i % 2 == 1 && Thread.currentThread().getName().equals("Odd")) {
            synchronized (lock) {
                System.out.println(Thread.currentThread().getName() + " - " + lock.i);
                lock.i++;
                try {
                    lock.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}}

This way you can share the lock and i among both the threads.