Is it mandatory to declare a variable volatile when I have a synchronized block to increment an int in Java?

Issue

I'm learning about concurrency in Java and I'm watching a YouTube video. It is an explanation about increment operation in a multi-threading environment. The solution for that could be a volatile variable or a synchronized block. But my question is, if I use a synchronized block do I need to use also volatile variable. Is not enough if I have a synchronized block? I see that the instructor makes the method synchronized and also makes the variable volatile. In my opinion is enough to make the method synchronized because when a thread enters the synchronized method it gets the value of the variable from main memory and when the thread exits the synchronized method it sends the value directly to main memory and it will be available for other threads. So I don't think it is mandatory to make the variable volatile.

This is the code:

public class ClientTest {
    public static void main(String[] args) {
        ExecutorService executorService = null;
        Counter counter = new Counter();

        try {
            executorService = Executors.newFixedThreadPool(2);

            Runnable task1 = () -> {
                for (int i = 1; i <= 20000; i++) {
                    counter.increment();
                }
            };

            Runnable task2 = () -> {
                for (int i = 1; i <= 80000; i++) {
                    counter.increment();
                }
            };

            executorService.submit(task1);
            executorService.submit(task2);
            executorService.awaitTermination(1, TimeUnit.SECONDS);

            System.out.println("Final counter value: " + counter.getCounter());
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }
    }
}


public class Counter {
    private int counter;
    public int getCounter() {
        return counter;
    }
    public synchronized void increment() {
        counter++;
    }
}

Is it mandatory to declare the counter variable volatile?

Solution

For correct behaviour of the Counter class, you will either need to mark the counter field volatile, or you need to make the getCounter() method synchronized as well, or alternatively, get rid of the Counter class and use AtomicInteger.

In the specific example of your code, it will probably work correctly, but that is - again probably - because you call awaitTermination on the executor service, and the submitted tasks will likely finish before the timeout expires. I think, but do not know for sure, this will establish a happens-before relationship between the tasks executed on the executor threads, and thus your main thread will be guaranteed to see the latest value of counter.

However, absent such a happens-before relationship, the main thread (or any other thread) is not guaranteed to see the latest value of counter, it could see any intermediate state, including the initial value of 0. To establish such a happens-before, you will need to mark the field volatile, or use synchronized on the method, as this will guarantee that the latest written value is read.

As an aside, calling awaitTermination without calling shutdown() on the executor service is not correct.

Answered By - Mark Rotteveel