Strange concurrent code behavior

Question

I'm currently in the process of learning Java concurrency. And I am very surprised by the way following code behaves.

import java.util.concurrent.*;

public class Exercise {
    static int counter = 0;

    static synchronized int getAndIncrement() {
        return counter++;
    }

    static class Improper implements Runnable {

        @Override
        public void run() {
            for (int i = 0; i < 300; i++) {
                getAndIncrement();
            }
        }
    }


    public static void main(String[] args) {
        ExecutorService executorService = Executors.newFixedThreadPool(3);
        for (int i = 0; i < 300; i++) {
            executorService.submit(new Improper());
        }
        executorService.shutdown();
        System.out.println(counter);
    }
}

Shouldn't it output 90000 all the time? Instead the result differs all the time.

Think, you just have to await termination of ExecutorService http://stackoverflow.com/questions/1250643/how-to-wait-for-all-threads-to-finish-using-executorservice — Viktor Ozerov, Sep 24 '13 at 08:18

score 25 · Accepted Answer · answered Sep 24 '13 at 08:18

25

executorService.shutdown() doesn't wait for the service to shut down. You need a call to awaitTermination.
you access the counter from the main method without locking. I think you would narrowly escape a data race if you waited for the executor service to shut down, but be warned that, in general, you must synchronize on all accesses of a shared variable, not just writes, to have any visibility guarantees from the Java Memory Model.

answered Sep 24 '13 at 08:18

Marko Topolnik

195,646
29
319
436

does it help to make counter volatile? – bvk256 Sep 24 '13 at 08:22
1

It does, but once again, it won't allow you to remove synchronization of the writing part. So you'll need both. – Marko Topolnik Sep 24 '13 at 08:25
2

@bvk256 - Given that you are already using `synchronized`, it would be better to write a synchronized getter method to fetch the final result. (Use of `synchronized` *and* a `volatile` variable is potentially inefficient.) – Stephen C Sep 24 '13 at 09:13
It will slow down the writing procedure with a doubled amount of memory barriers. On the other hand, it will speed up the reading due to a *halved* amount of memory barriers. – Marko Topolnik Sep 24 '13 at 09:19

score 3 · Answer 2 · answered Sep 24 '13 at 08:19

You need to make sure that all tasks have had time to terminate. Use awaitTermination

public static void main(String[] args) throws InterruptedException {
    ExecutorService executorService = Executors.newFixedThreadPool(3);
    for (int i = 0; i < 300; i++) {
        executorService.submit(new Improper());
    }
    executorService.shutdown();
    executorService.awaitTermination(2, TimeUnit.SECONDS);
    System.out.println(counter);
}

score 2 · Answer 3 · answered Sep 24 '13 at 08:18

2

You don't wait for all your submited tasks to terminate, see the javadoc for ExecutorService.html#shutdown. So gettting an arbitrary output each time is the expected behabiour.

answered Sep 24 '13 at 08:18

A4L

17,353
6
49
70

Strange concurrent code behavior

3 Answers3