When I use volatile in project, why the code below shows different result?

Question

first:

public class VolatileTest{
    public volatile int inc = 0;
    public void increase(){
        inc++;
    }

    public static void main(String[] args) {
        VolatileTest test = new VolatileTest();
        for(int i = 0 ; i < 2 ; i ++){
            new Thread(){
                public void run(){
                    for(int j = 0 ; j < 1000 ; j++)
                        test.increase();
                }
            }.start();
        } 
        while(Thread.activeCount() > 1)Thread.yield();
        System.out.println(test.inc);
    }
}

second:

public class VolatileTest{
    public volatile int inc = 0;
    public void increase(){
        inc++;
    }

    public static void main(String[] args) {
        VolatileTest test = new VolatileTest();
        new Thread(){
            public void run(){
                for(int j = 0 ; j < 1000 ; j++)
                    test.increase();
            }
        }.start();
        new Thread(){
            public void run(){
                for(int j = 0 ; j < 1000 ; j++)
                    test.increase();
            }
        }.start();
        while(Thread.activeCount() > 1)Thread.yield();
        System.out.println(test.inc);
    }
}

The first one uses a for and the second one doesn't, and that is the only difference, but the first one gets the result smaller than 2000, the second gets the result equals to 2000, why?

Because concurrency, try with `public synchonized void increase(){inc++;}` — David Pérez Cabrera, Mar 20 '18 at 10:27
what does `while(Thread.activeCount() > 1)Thread.yield();` mean? what did you want to achieve with it? — Andrew Tobilko, Mar 20 '18 at 10:39
I guess it should do the same as a `while(!workIsDone) //don't terminate` loop with `workIsDone` being some boolean flag to signal that all threads are done with their work. It's probably not the most efficient approach as the method only returns an estimate on the number of threads which might just be wrong as the threads could not be finished initializing yet so they don't show up. You could just save a reference to the threads and do a `Thread.join()` call which is just smarter. — Ben, Mar 20 '18 at 10:40

Ravindra Ranwala · Answer 1 · 2018-03-20T10:49:48.950

3

Consider this operation that you do inside the increase method. You first read the existing value, then increment it and write it back. There are several instructions here and that can be interrupted. The reason behind getting a value which is < 2000 is because of the race condition. Using the keyword volatile does not guarantee the atomicity. To guarantee atomicity you have to use a lock. Try this out.

private final Object lock = new Object();

public void increase() {
    synchronized (lock) {
        inc++;
    }

}

Another alternative is to use AtomicInteger here. So your code will now look like this.

public AtomicInteger inc = new AtomicInteger(0);
public void increase() {
    inc.incrementAndGet();
}

This also guarantees the atomicity as the name implies.

edited Mar 20 '18 at 10:49

answered Mar 20 '18 at 10:34

Ravindra Ranwala

20,744
6
45
63

You could also make the `increase` method itself synchronized to just use the instance lock instead if you don't want to use a custom lock. – Ben Mar 20 '18 at 10:38
yes that is another option. – Ravindra Ranwala Mar 20 '18 at 10:48

xingbin · Answer 2 · 2018-03-20T11:19:51.577

The result 2000 of second test is not guranteed by jls, you can let the thread sleep sometime before increment to make it "break" more easily:

public void increase(){
    try {
        Thread.sleep(20);
    } catch (Exception e) {

    }
    inc++;
}

You might get:

1997
1999

or some other unpredict results.

volatile can gurantee the changes to a variable are always visible to other threads, but can not gurantee the actions on this variable is atomic.

Suppose i = 1, thread1 and thread2 might read 1 at the same time, and increment it to 2, then write back, which leads to wrong result.

score 1 · Accepted Answer · answered Mar 20 '18 at 13:37

That’s just coincidence. Both variants are equally broken, but the second defines two distinct classes doing the same thing, so before starting the second thread, this additional class has to be loaded, verified and initialized. This overhead gives the first thread a headstart, raising the chance of completing entirely before the second even starts.

So the race condition does not materialize then, but since this execution is not guaranteed, it’s still a broken program containing the possibility of data races. Running the same program in an environment with faster class loading/initialization or an ahead-of-time strategy may exhibit the same behavior as with the first variant.

Note that likewise, it’s not guaranteed that the first variant will experience lost updates. It may still happen that starting the second thread is slow enough to allow the first one to complete without data races. Even if both threads run, the system’s thread scheduling policy may change the likelihood of experiencing lost updates. Also, the entire loop could get optimized to a single increment by 1000, that would not contradict the requirements for volatile variables, even if the current version of the HotSpot JVM doesn’t do that.

I test by print the test.inc in the second code's second thread and then I get the 1000. It is right. — jt z, Mar 22 '18 at 15:22

When I use volatile in project, why the code below shows different result?

3 Answers3