Different behavior when implementing Runnable instead of extending Thread
So here's the code. Basically if we change the ReadCalculation and Calculator classes to extend Thread instead of implementing Runnable we would need to instantiate these classes and pass them to a new thread object or just call start() on them.
Calculator calc = new Calculator();
new ReadCalculation(calc).start();
new ReadCalculation(calc).start();
calc.start();
Nothing special so far.. But when you execute this tiny program, there's a huge chance that your threads will stay blocked "Waiting for calculation..." if we're going over the Runnable implementation over extending the Thread class.
If we're extending the Thread class instead of implementing Runnable the behavior is correct without any sign of race condition. Any ideas on which could be the source of this behavior?
public class NotifyAllAndWait {
public static void main(String[] args) {
Calculator calc = new Calculator();
Thread th01 = new Thread(new ReadCalculation(calc));
th01.start();
Thread th02 = new Thread(new ReadCalculation(calc));
th02.start();
Thread calcThread = new Thread(calc);
calcThread.start();
}
}
class ReadCalculation implements Runnable {
private Calculator calc = null;
ReadCalculation(Calculator calc) {
this.calc = calc;
}
@Override
public void run() {
synchronized (calc) {
try {
System.out.println(Thread.currentThread().getName() + " Waiting for calculation...");
calc.wait();
} catch (InterruptedException e) {
e.printStackTrace();
}
System.out.println(Thread.currentThread().getName() + " Total: " + calc.getTotal());
}
}
}
class Calculator implements Runnable {
private int total = 0;
@Override
public void run() {
synchronized(this) {
System.out.println(Thread.currentThread().getName() + " RUNNING CALCULATION!");
for(int i = 0; i < 100; i = i + 2){
total = total + i;
}
notifyAll();
}
}
public int getTotal() {
return total;
}
}
In the implements Runnable version, at least, you do nothing to ensure that the ReadCalculation threads reach the wait() before the Calculator thread enters its synchronized block. If the Calculator thread enters its synchronized block first, then it will call notifyAll() before the ReadCalculation threads have called wait() . And if that happens, then the notifyAll() is a no-op, and the ReadCalculation threads will wait forever. (This is because notifyAll() only cares about threads that are already waiting on an object; it does not set any sort of indicator on the object that could be detected by subsequent calls to wait() .)
To fix that, you could add a property to Calculator that can be used to check for done-ness, and only call wait() if the Calculator is not done:
if(! calc.isCalculationDone()) {
calc.wait();
}
(Note that, in order to completely avoid the race condition, it's important that the entire if -statement be inside the synchronized block, and that Calculator set this property inside the synchronized block that calls notifyAll() . Do you see why?)
(By the way, Peter Lawrey's comment that "a thread can easily to your 100 iterations before the other threads have even started" is highly misleading, since in your program the 100 iterations all happen after Calculator has entered its synchronized block. Since the ReadCalculation threads are blocked from entering their synchronized blocks and calling calc.wait() while Calculator is in its synchronized block, it shouldn't matter whether this is 1 iteration, 100 iterations, or 1,000,000 iterations, unless it has interesting optimization effects that could change the timing of the program before that point.)
You haven't posted the entire extends Thread version, but if I understand correctly what it looks like, then it actually still has the same race condition. However, it's in the nature of race conditions that minor changes can massively affect the likelihood of misbehavior. You still need to fix the race condition even if it never seems to actually misbehave, because it's almost certain that it will misbehave occasionally if you run the program enough times.
I don't have a good explanation for why the misbehavior seems to happen much more often with one approach than the other; but as user1643723 comments above, the extends Thread approach implies that lots of code other than yours is also likely to lock on your Calculator instance; and this could well have an effect of some sort. But honestly, I don't think it's worth worrying too much about the reasons why a race condition might cause a misbehavior more often or less often; we have to fix it regardless, so, end of story.
Incidentally:
Above, I used if(! calc.isCalculationDone()) ; but it's actually a best practice to always wrap calls to wait() in an appropriate while -loop, so really you should write while(! calc.isCalculationDone()) . There are two major reasons for this:
In nontrivial programs, you don't necessarily know why notifyAll() was called, or even if you do, you don't know whether that reason is still applicable by the time the waiting thread has actually woken up and regained the synchronized -lock. It makes it much easier to reason about the correctness of your notify() / wait() interactions if you use the while(not_ready_to_proceed()) { wait(); } while(not_ready_to_proceed()) { wait(); } structure to express the idea of wait_until_ready_to_proceed() , rather than just writing wait() and trying to make sure that nothing will ever cause it to return while we're not ready.
On some operating systems, sending a signal to the process will wake up all threads that are wait() -ing. This is called a spurious wakeup; see "Do spurious wakeups actually happen?" for more information. So a thread may get woken up even if no other thread called notify() or notifyAll() .
The for -loop in Calculator.run() shouldn't be in the synchronized block, because it doesn't require any synchronization, so the contention is not needed. In your small program, it doesn't actually make a difference (since none of the other threads actually has anything to do at that point anyway), but the best practice is always to try to minimize the amount of code inside synchronized blocks.
When you perform a wait() this needs to be in a loop after a state change you performed the notify() block. eg
// when notify
changed = true;
x.notifyAll();
// when waiting
while(!changed)
x.wait();
If you don't do this you will run into issue such as wait() waking spuriously or notify() being lost.
Note: a thread can easily to your 100 iterations before the other threads have even started. It is possible that pre-creating the Thread object makes enough of a difference to the performance to change the outcome in your case.