Not getting expected result when updating AtomicInteger variable in multiple threads

Question

In this code I'm using 10 threads updating an AtomicInteger variable. I expect the final result of Counter.getInstance().holder.n to be 1000000, but it prints out random number like 991591. What's wrong with my code?

public class Test {


    public static void main(String[] args) {

        List<Thread> list = new ArrayList<Thread>();
        for (int i = 0; i < 10; i++) {
            list.add(new Thread() {

                public void run() {
                    for (int i = 0; i < 100000; i++) {
                        Counter.getInstance().holder.n.incrementAndGet();
                    }
                }
            });
        }
        for (Thread thread : list) {
            thread.start();
        }

        try {
            Thread.sleep(10000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        System.out.println(Counter.getInstance().holder.n);


    } 
} 

class Counter {
    private static Counter counter;
    Holder holder = new Holder();


    public static Counter getInstance() {
        if (counter == null) {
            counter = new Counter();
        }
        return counter;
    }
    class Holder {
        AtomicInteger n = new AtomicInteger(0);
    } 
}

Your Counter singleton is not thread-safe, and sleeping 10 seconds doesn't guarantee that all threads have finished running. Use a simple static variable for your atomic integer, and call thread.join() on all the threads (in an additional loop) to wait for their termination. — JB Nizet, Nov 29 '19 at 08:43
Can you explain why the Singleton is not thread-safe? and how to make it thread-safe? — zonyang, Nov 29 '19 at 08:57
Because two threads can concurrently call getInstance(), both see that counter is null, and both create a new Counter. And since there is no synchronization whatsoever, even if one thread has already set the static counter to a non-null value, the other thread has no guarantee to see it. To make it thread-safe: just use `static final AtomicInteger N = new AtomicInteger(0)` — JB Nizet, Nov 29 '19 at 09:05

Vladimir Pligin · Accepted Answer · 2019-11-29T13:57:29.317

You have two major concurrent issues here:

You don't wait for every Thread to finish work correctly. There are multiple ways to achieve that, the simplest is to use Thread.join().
Your singleton implementation doesn't seem correct. I suppose you intended to implement it with an inner class. It seems that this answer can help to understand what's happening here.

Here is the implementation that seems more or less correct.

class Test {
    public static void main(String[] args) throws InterruptedException {

        List<Thread> list = new ArrayList<Thread>();
        for (int i = 0; i < 10; i++) {
            list.add(new Thread() {

                public void run() {
                    for (int i = 0; i < 100000; i++) {
                        Counter.getInstance().n.incrementAndGet();
                    }
                }
            });
        }
        for (Thread thread : list) {
            thread.start();
        }

        for (Thread thread : list) {
            thread.join();
        }

        System.out.println(Counter.getInstance().n);
    }
}

class Counter {
    public AtomicInteger n = new AtomicInteger(0);

    public static Counter getInstance() {
        return Holder.instance;
    }
    private static class Holder {
        private static final Counter instance = new Counter();
    }
}

You can use something like CountDownLatch as well. For example:

final int count = 10;
CountDownLatch latch = new CountDownLatch(count);
List<Thread> list = new ArrayList<Thread>();
for (int i = 0; i < count; i++) {
    list.add(new Thread() {

        public void run() {
            for (int i = 0; i < 100000; i++) {
                Counter.getInstance().n.incrementAndGet();
            }
            latch.countDown();
        }
    });
}
for (Thread thread : list) {
    thread.start();
}

latch.await();

System.out.println(Counter.getInstance().n);

Not getting expected result when updating AtomicInteger variable in multiple threads

1 Answers1