Why volatile plus synchronized doesn't work?

Question

I'm trying to understand concurrency in java. I know about synchronized, that it creates a monitor on object, and after that another Threads can't do operations with this object. Volatile - is about processor cache, and if i use it, all threads don't create a copy of object. So, in my mind if i run this code, i will get the correct counter value (40000). But i get incorrect!

public class Main {

private static volatile Integer counter = 0;

public static void main(String[] args) throws InterruptedException {
    Thread thread = new Counter();
    Thread thread2 = new Counter();
    thread.start();
    thread2.start();
    thread.join();
    thread2.join();
    System.out.println(counter);
}


public static class Counter extends Thread{
    @Override
    public void run() {
        for (int i = 0; i < 20000; i++) {
            synchronized (counter){
                counter++;
            }
        }
    }
}
}

But if i use syncronized method, i will get correct result:

public class Main {

private static volatile Integer counter = 0;

public static void main(String[] args) throws InterruptedException {
    Thread thread = new Counter();
    Thread thread2 = new Counter();
    thread.start();
    thread2.start();
    thread.join();
    thread2.join();
    System.out.println(counter);
}

public synchronized static void increment(){
    counter++;
}

public static class Counter extends Thread{
    @Override
    public void run() {
        for (int i = 0; i < 20000; i++) {
            increment();

        }
    }
}
}

So question - why synchronized doesn't work on Integer object??

Answer 1

You are misunderstanding several things:

1. volatile is not about CPU cache. All modern processors employ multi-level CPU caches, that are completely transparent to the application, thus the app does not have to care whether they are fetching from L1, L2,L3, RAM, etc. This is accomplished by the CPU implementing some cache-coherence protocol, such as MESI or its variations. So what does volatile do then ? It prevents certain compiler optimizations. For instance if you read the value of a variable once, without volatile the compiler may optimize away any subsequent reads of this variable, because it assumes that it could have not possibly changed. With volatile it will not remove those additional reads.

2. The synchronized keyword uses some object as a lock, but in your case you are changing that object. So lets assume that thread-1 locks on Integer(0) and then due to autoboxing, the ++ operation changes the object to Integer(1). And your second thread is free to acquire that lock, because it's not held by anyone.

3. It's very bad idea to lock/synchronize on strings (because they can be interned) or Boolean, or Integer, etc. For instance strings objects can be interned and you can have several parts of the program to try to acquire lock on the same instance, although from their point of view it should be a different instance. The boolean true/false are cached. The auto-boxed integers from -128 to +127 are cached, so if you may experience the same issue as with the interned strings.

So for synchronization it's much better to use proper locks and avoid the synchronized word. I'd even go further and say that it was a mistake in java.

Answer 2

You are using a synchronized block on a non-final field. When you use counter++, since Integer is immutable, then the new reference will be assigned to the counter. Please check this answer for more details - Synchronization of non-final field

You can use ReentrantLock instead of synchronized, but another problem - you are not using the atomic operation on the volatile field. You should use AtomicInteger instead

import java.util.concurrent.atomic.AtomicInteger;

public class Main {

    private static final AtomicInteger counter = new AtomicInteger(0);

    public static void main(String[] args) throws InterruptedException {
        Thread thread = new Counter();
        Thread thread2 = new Counter();
        thread.start();
        thread2.start();
        thread.join();
        thread2.join();
        System.out.println(counter);
    }


    public static class Counter extends Thread {
        @Override
        public void run() {
            for (int i = 0; i < 20000; i++) {
                counter.getAndIncrement();
            }
        }
    }
}

For the reference, with lock you do not need volatile:

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class Main {

    private static int counter = 0;
    private static final Lock lock = new ReentrantLock();

    public static void main(String[] args) throws InterruptedException {
        Thread thread = new Counter();
        Thread thread2 = new Counter();
        thread.start();
        thread2.start();
        thread.join();
        thread2.join();
        System.out.println(counter);
    }


    public static class Counter extends Thread {
        @Override
        public void run() {
            for (int i = 0; i < 20000; i++) {
                lock.lock();
                counter++;
                lock.unlock();
            }
        }
    }
}