Java volatile keyword not working as expected

Question

I am learning volatile variable. I know what volatile does, i wrote a sample program for Volatile variable but not working as expected.

Why is the final value of the "count" coming sometime less then 2000. I have used volatile hence the system should not cache "count" variable and the value should always be 2000.

When I used synchronized method it work fine but not in the case of volatile keyword.

public class Worker {

private volatile int count = 0;
private int limit = 10000;

public static void main(String[] args) {
    Worker worker = new Worker();
    worker.doWork();
}

public void doWork() {
    Thread thread1 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < limit; i++) {

                    count++;

            }
        }
    });
    thread1.start();
    Thread thread2 = new Thread(new Runnable() {
        public void run() {
            for (int i = 0; i < limit; i++) {

                    count++;

            }
        }
    });
    thread2.start();

    try {
        thread1.join();
        thread2.join();
    } catch (InterruptedException ignored) {}
    System.out.println("Count is: " + count);
}
}

Thank You in advance !

Answer 1

When you do count++, that's a read, an increment, and then a write. Two threads can each do their read, each do their increment, then each do their write, resulting in only a single increment. While your reads are atomic, your writes are atomic, and no values are cached, that isn't enough. You need more than that -- you need an atomic read-modify-write operation, and volatile doesn't provide that.

Answer 2

count++ is basically this:

// 1. read/load count
// 2. increment count
// 3. store count
count = count + 1;

individually the first and third operation are atomic. All 3 of them together are not atomic.

Answer 3

i++ is not atomic in Java. So two threads may concurrently read, both calculate +1 to be the same number, and both store the same result.

Compile this using javac inc.java:

public class inc {
    static int i = 0;
    public static void main(String[] args) {
        i++;
    }
}

Read the bytecode using javap -c inc. I've trimmed this down to just show the function main():

public class inc {
  static int i;

  public static void main(java.lang.String[]);
    Code:
       0: getstatic     #2                  // Field i:I
       3: iconst_1
       4: iadd
       5: putstatic     #2                  // Field i:I
       8: return
}

We see that increment (of a static int) is implemented using: getstatic, iconst_1, iadd and putstatic.

Since this is done with four instructions and no locks, there can be no expectation of atomicity. Also worth noting that even if this were done with 1 instruction, we may be out of luck (quote from user "Hot Licks" comment in this thread):

Even on hardware that implements an "increment storage location" instruction, there's no guarantee that that's thread-safe. Just because an operation can be represented as a single operator says nothing about it's thread-safety.

---

If you actually wanted to solve this, you could use AtomicInteger, which has an atomicity guarantee:

final AtomicInteger myCoolInt = new AtomicInteger(0);
myCoolInt.incrementAndGet(1);

Answer 4

When you used synchronized method it was working as expected because it ensures that if one of the threads executes that method, the execution of the other caller threads is suspended until the currently executing one exits the method. In this case the whole read-increment-write cycle is atomic.

From the tutorial:

First, it is not possible for two invocations of synchronized methods on the same object to interleave. When one thread is executing a synchronized method for an object, all other threads that invoke synchronized methods for the same object block (suspend execution) until the first thread is done with the object.

Second, when a synchronized method exits, it automatically establishes a happens-before relationship with any subsequent invocation of a synchronized method for the same object. This guarantees that changes to the state of the object are visible to all threads.

When you use volatile (as it is explained by others) this cycle is not atomic as using this keyword does not ensure that there will be no other write on the variable on other threads between the get and the increment steps on this thread.

For atomic counting rather than the synchronized keyword, you could use e.g. an AtomicInteger:

public class Worker {
    private AtomicInteger count = new AtomicInteger(0);
    private int limit = 10000;

    public static void main(String[] args) {
        Worker worker = new Worker();
        worker.doWork();
    }

    public void doWork() {
        Thread thread1 = new Thread(new Runnable() {
            public void run() {
                for (int i = 0; i < limit; i++)
                    count.getAndIncrement();
            }
        });
        thread1.start();
        Thread thread2 = new Thread(new Runnable() {
            public void run() {
                for (int i = 0; i < limit; i++)
                    count.getAndIncrement();
            }
        });

        thread2.start();

        try {
            thread1.join();
            thread2.join();
        } catch (InterruptedException ignored) {
        }
        System.out.println("Count is: " + count);
    }
}

Here getAndIncrement() ensures the atomic read-increment-set cycle.

Answer 5

Memory Visibility and Atomicity are two different but common problem in Multithreading. When you use synchronized keyword, it ensures both by acquiring the locks. Whereas volatile only solves the memory visibility issue. In his book Concurrency in practice, Brain Goetz explains when you should use volatile.

1. Writes to the variable do not depend on its current value, or you can ensure that only a single thread ever updates the value;
2. The variable does not participate in invariants with other state variables;
3. Locking is not required for any other reason while the variable is being accessed.

Well, in your case look at the operation count++ which isn't atomic.

Answer 6

for (int i = 0; i < limit; i++) {
    count++;
}

here count++ has three serial operations read, increment then write, thread 1 may operate initially and then thread scheduler may shift to thread 2 after reading the count value, the thread 2 may modify the count, after thread scheduler shifts to thread 1 it has the previous value it read before and so here there is a race condition(which we have to check and act) and but the operation is not done as atomically, u better use AtomicInteger class else increment it as a atomic unit by synchronising