The main thread randomly doesn't reach end (trying to sum natural numbers in concurrent threads)

Question

I have the following code to find the sum of natural numbers from 1 to 5000. It's a simple exercise to practice concurrency.

public static void main(String[] args) throws InterruptedException {
        final int[] threadNb = new int[] {5};
        final Integer[] result = new Integer[1];
        result[0] = 0;
        List<Thread> threads = new LinkedList<>();
        IntStream.range(0, threadNb[0]).forEach(e -> {
            threads.add(new Thread(() -> {
                int sum = 0;
                int idx = e * 1000 + 1;
                while (!Thread.interrupted()) {
                    if (idx <= (e + 1) * 1000) {
                        sum += idx++;
                    } else {
                        synchronized(result) {
                            result[0] += sum;
                            System.err.println("sum found (job " + e + "); sum=" + sum + "; result[0]=" + result[0] + "; idx=" + idx);
                            Thread.currentThread().interrupt();
                        }
                    }
                }
                synchronized(result) {
                    System.err.println("Job " + e + " done. threadNb = " + threadNb[0]);
                    threadNb[0]--;
                    System.err.println("threadNb = " + threadNb[0]);
                }
            }));
        });
        threads.forEach(Thread::start);
        //noinspection StatementWithEmptyBody
        while(threadNb[0] > 0);
        System.out.println("begin result");
        System.out.println(result[0]);
        System.out.println("end result");
    }

Sometimes, when I run the code, the last 3 System.out.println() are not displayed. If I put a statement in the while(threadNb[0] > 0), like another System.out.println(), my problem never happen again.

Can anyone explain me this behaviour?

Thanks in advance for any help

Nathan Hughes · Accepted Answer · 2015-09-11T17:18:02.650

Nothing about how the threadNb variable is declared tells the JVM that it needs to make updates to it visible to other threads. At what point updates to your variable become visible to other threads is entirely up to the JVM implementation, it can make them visible or not depending on circumstances. Also, the JIT is free to re-order or optimize away code if it thinks it can get away with it, and it bases its decisions on the visibility rules. So it's hard to say exactly what happens here because the behavior is left unspecified by the Java language specs, but definitely you're having a problem where your worker threads' updates are usually not getting seen by the main thread.

If you replace the arrays with AtomicInteger then the updates are guaranteed to become visible to other threads. (Volatile works too, but AtomicInteger is preferred. In order to use volatile you have to make the variable an instance or class member.) If you save the updated values in a local variable then you don't need synchronization:

import java.util.*;
import java.util.stream.*;
import java.util.concurrent.atomic.*;    
public class SumNumbers {
    public static void main(String[] args) throws InterruptedException {
        AtomicInteger threadNb = new AtomicInteger(5);
        AtomicInteger result = new AtomicInteger(0);
        List<Thread> threads = new LinkedList<>();
        IntStream.range(0, threadNb.intValue()).forEach(e -> {
            threads.add(new Thread(() -> {
                int sum = 0;
                int idx = e * 1000 + 1;
                while (!Thread.currentThread().isInterrupted()) {
                    if (idx <= (e + 1) * 1000) {
                        sum += idx++;
                    } else {
                        int r = result.addAndGet(sum);
                        System.out.println("sum found (job " + e + "); sum=" 
                        + sum + "; result=" + r 
                        + "; idx=" + idx);
                        Thread.currentThread().interrupt();
                    }
                }
                System.out.println("Job " + e + " done.");
                int threadNbVal = threadNb.decrementAndGet();
                System.out.println("Job " + e + " done, threadNb = " + threadNbVal);
            }));
        });
        threads.forEach(Thread::start);
        //noinspection StatementWithEmptyBody
        while(threadNb.intValue() > 0);
        System.out.println("result=" + result.intValue());
    }
}

where you can see the updates become visible.

Busy waiting is not preferred since it wastes CPU cycles. You do see it in lock-free programming, but it is not a good thing here. Thread#join will work or you can use a CountdownLatch.

Be aware that using Thread#interrupted() clears the interrupted flag. Usually it's used when you're about to throw InterruptedException, otherwise it's better to use Thread.currentThread().isInterrupted(). It doesn't hurt anything in this specific case because the while loop test is the only thing using the flag, so whether it gets cleared is irrelevant.

score 2 · Answer 2 · answered Sep 11 '15 at 12:54

The most likely explanation is that the compiler optimizes your code so that the value of threadNb[0] is cached. Therefore, the main thread may not see updates done by other threads. Making your counter volatile can help to solve this problem.

However, the current approach with a busy wait is generally not the best solution. You should use the join() method of the class Thread to let your main thread wait till each of them has ended.

For example:

for(Thread t: threads) {
    try{
        t.join();
    } catch(InterruptedException e) {}
}

The main thread randomly doesn't reach end (trying to sum natural numbers in concurrent threads)

2 Answers2