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
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.