Eliminate race condition in ExecutorService with newSingleThreadExecutor

Question

I wrote the following program to understand racing:

import java.util.concurrent.*;

class RaceCount
{
    static int count = 0;

    public static void main(String [] args)
    {        
        ExecutorService executor = Executors.newSingleThreadExecutor();

        for (int i = 0; i < 1000; i++)
        {
            executor.submit(new Callable<String>() {
                public String call() throws Exception {
                    count++; return "Incremented";
                }
            });
        }
        executor.shutdown();
        System.out.println(count);
    }
}

Obviously, the count was much less than 1000. So, I changed the call() method signature to:

public synchronized String call() throws Exception {

But, the result was still less than 1000. If I use newFixedThreadExecutor(1000) instead of newSingleThreadExecutor, then I get the expected 1000 even if the call() method is not prefixed with synchronized keyword.
So, my queries are:
1. How to synchronize the threads in the case of newSingleThreadExecutor?
2. Why no synchronization is required, when newFixedThreadExecutor is used?

See https://stackoverflow.com/questions/53169939/executor-suitable-for-non-thread-safe-code — Raedwald, Dec 04 '18 at 06:54

ernest_k · Accepted Answer · 2018-12-04T07:00:45.557

Your problem is not due to a race condition. It's occurring simply because executor.shutdown() does not wait for a full shut down before returning.

This is from javadocs of java.util.concurrent.ExecutorService.shutdown():

...
This method does not wait for previously submitted tasks to complete execution. Use awaitTermination to do that.

In other words, System.out.println(count) is running before some tasks have run (although it necessarily runs after all tasks have been submitted).

I made a small change to your code to make this fact evident:

public static void main(String[] args) {
    ExecutorService executor = Executors.newSingleThreadExecutor();

    for (int i = 0; i < 1000; i++) {
        int e = i;
        executor.submit(new Callable<String>() {
            public String call() throws Exception {
                System.out.println("Executing " + e);
                count++;
                return "Incremented";
            }
        });
    }
    executor.shutdown();
    System.out.println("Count: " + count);
}

And the output goes like this:

...
Executing 835
Executing 836
Executing 837
Count: 837     <----- Printed before all tasks are run
Executing 838
Executing 839
Executing 840
Executing 841
...

Which clearly shows that tasks continue running after you've read the count variable.

If you need to be sure that tasks are executed before you read the updated value, then you may need to use awaitTermination, as follows:

executor.shutdown();
executor.awaitTermination(3, TimeUnit.SECONDS); //Pick an appropriate timeout value
System.out.println("Count: " + count);

score 1 · Answer 2 · answered Dec 04 '18 at 07:03

the part about shutdown is only half of the solution. "public synchronized String call()" this synchronizes the call so that only one thread can at the same time execute the call of one instance, but with "executor.submit(new Callable() " you have 1000 instances of your call. So in effect no synchronisation. You can change this to "Callable call = new Callable()..." outside the loop. And "executor.submit(call);" inside so that you have one call instance that is synchronized. Or change from "int i" to "AtomicInteger i" and from ++i to i.incrementAndGet();

Eliminate race condition in ExecutorService with newSingleThreadExecutor

2 Answers2