Volatile key was not working as expected

Question

Am writing sample app to understand volatile behavior. According to me the value of this variable should change to 50 but the output I'm getting is 10.

Main method:

public class Volatile {
    public static void main(String[] args) {
        ShareObj so = new ShareObj();
        Thread1 t1 =  new Thread1(so);
        Thread2 t2 =  new Thread2(so);
        t1.start();
        t2.start();
        System.out.println(so.a);
    }
}

Classes:

class ShareObj {
    public volatile int a =10; 
}

class Thread1 extends Thread {
    ShareObj so;
    Thread1(ShareObj so) {
        this.so  = so;
    }
    public void run() {
        so.a += 10;
        System.out.println(so.a);
    }
}

class Thread2 extends Thread {
    ShareObj so;
    Thread2(ShareObj so) {
        this.so=so;
    }
    public void run() {
        so.a+=10;
        System.out.println(so.a);
    }
}

The output I'm expecting is 50 but it is 10.

Any suggestions?

For one thing, your threads probably haven't run a single statement by the time `main` runs `System.out.println(so.a);` (because starting a thread is slower than printing something) — user253751, Jun 23 '15 at 11:37
Note in this example you'll likely get the same result with or without volatile, in practice. — user253751, Jun 23 '15 at 11:59
Also, realistically, if you *ever* think you need to use `volatile`, then there's a 99% chance you have a race condition somewhere (unrelated to `volatile`). — user253751, Jun 23 '15 at 12:00

score 4 · Answer 1 · answered Jun 23 '15 at 11:40

First you need to wait for both threads to finish, before printing their results.

...
t1.start();
t2.start();
t1.join(); // this will make main thread to wait untill thread is finished
t2.join();
.....

Currently, you are stating two threads, but before any of them could change volatile value, your main thread is pring value and exiting.

Docs are here.

score 3 · Accepted Answer · edited May 23 '17 at 11:51

You have two issues:

As @immibis and @Beri already pointed out, your System.out.println(so.a); in your main method will probably run before your threads are finished.
You start a at 10 and have two threads which increment it by 10 each, so you should expect 30 instead of 50.

After changing your main method to

public static void main(String[] args) throws InterruptedException {
    // TODO Auto-generated method stub
    ShareObj so = new ShareObj();
    Thread1 t1 = new Thread1(so);
    Thread2 t2 = new Thread2(so);
    t1.start();
    t2.start();
    t1.join();
    t2.join();
    System.out.println(so.a);
}

the code prints out 30 as expected (as last line; note that both of the threads might output 20 or 30, so you shouldn't rely on the first two outputs).

I would also like to recommend @user3707125's answer, pointing out that volatile might still not produce the expected results (when you have more threads and/or more incrementing steps) - cf. also this Stack Overflow question.

score 2 · Answer 3 · answered Jun 23 '15 at 11:53

I am considering that the code you provided is simplified, and you used debug execution in order to assure that both threads completed their job when System.out.println call was made, otherwise you should use Thread.join before printing the result.

Now regarding volatile keyword. volatile means that there would be no thread cache for this variable. However it doesn't mean that actions with this variable will be performed atomically.

The code so.a += 10 if simplified means nothing more than so.a = so.a + 10, this operation is performed in two steps by the JVM (lets omit the field access for simplicity):

Compute so.a + 10
Assign so.a to the result of computation

Now lets analyze how it may affect the execution (image that next case occurred):

thread_1: compute and put on stack (so.a + 10) => 0 + 10 => 10 (x)
thread_2: compute and put on stack (so.a + 10) => 0 + 10 => 10 (y)
thread_1: assign so.a from stack => so.a = x => so.a = 10
thread_2: assign so.a from stack => so.a = y => so.a = 10
main_thread: print so.a => print(10)

So it is not safe to use the code that your wrote, even with the volatile keyword.

If you want to verify this case change the thread code to something like:

for (int i = 0; i < 1000000000; i++) {
    so.a += 1;
}

And you would see that the result is always different and almost never 2kkk.

Volatile key was not working as expected

3 Answers3