Sequential Thread Execution using wait() and notifyAll()

Problem Statement:


T1, T2 to Tn threads prints numbers up to N such that each threads
prints number in circular sequential fashion. T1 prints 1, T2 prints
2, T3 prints 3 and T1 prints 4 again following strict ordering. This
implementation would allow us to control any number of threads to do
sequential ordered execution. It uses wait() and notifyAll() methods
to signal threads. I am using HashMap to store sequence I would like
to achieve and this can allow me to add N threads as long as I can
control turns for each threads.

Please suggest any improvements.

class SignalThread implements Runnable {

    volatile Integer i = 1;

    volatile String turn = "1";

    Map sequence = new HashMap();

    WaitNotifySignal waitNotifySignal;

    public SignalThread(WaitNotifySignal waitNotifySignal) {
        this.waitNotifySignal = waitNotifySignal;
    }

    @Override
    public void run() {
        while (i.intValue()  sequence) {
        this.sequence = sequence;
    }

    private String getNextTurn(String currentTurn) {
        return sequence.get(currentTurn);
    }
}

And:

```
/**
* Common Wait Notify Signal class
*
* @author manishdevraj
*
*/

class WaitNotifySignal {

Object monitorObject = new Object();

boolean wasSignalled = false;

public void doWait() {
synchronized (monitorObject) {
while (!wasSignalled) {
try {
monitorObject.wait();
} catch (InterruptedException e) {
}
}
// clear signal and continue running.
wasSignalled = false;
}
}

public void doNotifyAll() {
synchronized (monitorObject) {
wasSignalled = true;
monitorObject.notify();
}
}
}

public class LinearCircularSignalT {

public static void main(String[] args) {

SignalThread signalThread = new SignalThread(new WaitNotifySignal());
Thread t1 = new

the first issue I see is that i++; is not thread safe. Instead use an AtomicInteger

second is that the thread gets into a spin lock when it is awake and it's not its turn. the solution to this is to do:

while(!Thread.currentThread().getName().equalsIgnoreCase(turn)) {
    waitNotifySignal.doWait();
}

but this has a race condition (turn changed after the test but before the wait->the changing thread has already called doNotifyAll() and deadlock occurs. (This is the reason why wait and notify have to be in the synchronized blocks)

This means that you need to synchronize the testing and the changing of turn, but if we do this then we don't need a special WaitNotifySignal class but a normal Object will suffice:

class SignalThread implements Runnable {

    final AtomicInteger index = new AtomicInteger(1);

    String turn = "1"; // synchronization will take care of visibility

    Map sequence = new HashMap();

    Object waitNotifySignal;

    public SignalThread(Object waitNotifySignal) {
        this.waitNotifySignal = waitNotifySignal;
    }

    @Override
    public void run() {
        int i = index.get();
        while (i <= 10) {
            synchronized(waitNotifySignal){
                while(!Thread.currentThread().getName().equalsIgnoreCase(turn)) {
                    try{
                        waitNotifySignal.wait();
                    }catch(InterruptedException e){
                        return;//interrupted means that the thread should stop
                    }
                }
            }

            System.out.println("Thread: "
                    + Thread.currentThread().getName() + " --- " + i);
            i = index.incrementAndGet();

            synchronized(waitNotifySignal){
                turn = getNextTurn(turn);
                waitNotifySignal.notifyAll();
            }
        }
    }

    //...
}

besides that your doNotifyAll() didn't. only one thread ever got out of the synchonized block in doWait, which means that deadlock would occur if more than one thread was waiting and the wrong thread got notified.

StackExchange Code Review Q#51486, answer score: 4