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

Question

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);
        }
    }
}

… and:

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?

Answer 1

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.

Answer 2

Atomic…

As mentioned in the correct Answer by Mark Rotteveel, an alternative to using synchronized/volatile is the use of the Atomic… classes.

I prefer the Atomic… approach because it makes very clear to the reader what resources you are concerned about protecting from multi-threaded access/manipulation. And using Atomic… makes quite clear how you intend to protect those resources.

Furthermore, using Atomic… often simplifies the code and the reader’s comprehension.

Properly shutting down executor service

That same Answer by Mark Rotteveel is also correct in pointing out that your code does not properly shut down the executor service. To do so properly, either:

• Use the boilerplate code in shutdownAndAwaitTermination method given on the Javadoc of ExecutorService, or…
• Use try-with-resources syntax to automatically close the executor service after it completes its tasks (within a day).

I prefer the second approach, try-with-resources.

Example code

Let's use both these concepts (Atomic… & auto-closing of executor service) in this revised code.

import java.util.Locale;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;

public class ClientTest
{
    public static void main ( String[] args )
    {
        final AtomicInteger counter = new AtomicInteger ( );

        try (
                ExecutorService executorService = Executors.newFixedThreadPool ( 2 ) ;
        )
        {
            Runnable task1 = ( ) -> {
                for ( int i = 1 ; i <= 20_000 ; i++ )
                {
                    counter.incrementAndGet ( );
                }
            };

            Runnable task2 = ( ) -> {
                for ( int i = 1 ; i <= 80_000 ; i++ )
                {
                    counter.incrementAndGet ( );
                }
            };

            executorService.submit ( task1 );
            executorService.submit ( task2 );
        }
        System.out.println ( "Final counter value: %,d".formatted ( counter.get ( ) ) );
    }
}

Final counter value: 100,000

I am concerned about our AtomicInteger named counter here being a local variable. I am not sure if being a local variable presents a visibility problem. I always use the Atomic… variables as a final instance member (or static) variable that is populated on its declaration line as a simple way to ensure thread-safety.