Sequential Thread Execution using wait() and notifyAll()

Question

Problem Statement:

T₁, T₂ to T_n threads prints numbers up to N such that each threads prints number in circular sequential fashion. T₁ prints 1, T₂ prints 2, T₃ prints 3 and T₁ 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<String, String> sequence = new HashMap<String, String>();

    WaitNotifySignal waitNotifySignal;

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

    @Override
    public void run() {
        while (i.intValue() <= 10) {
            if (Thread.currentThread().getName().equalsIgnoreCase(turn)) {
                System.out.println("Thread: "
                        + Thread.currentThread().getName() + " --- " + i);
                i++;
                turn = getNextTurn(turn);
                waitNotifySignal.doNotifyAll();
                waitNotifySignal.doWait();
            }
        }
    }

    public void setTurnSequence(Map<String, String> 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 Thread(signalThread);
        t1.setName("1");
        Thread t2 = new Thread(signalThread);
        t2.setName("2");
        Thread t3 = new Thread(signalThread);
        t3.setName("3");
        Map<String, String> sequence = new HashMap<String, String>();
        sequence.put("1", "2");
        sequence.put("2", "3");
        sequence.put("3", "1");
        signalThread.setTurnSequence(sequence);
        t1.start();
        t2.start();
        t3.start();

    }
}

actually you could have moved the index.incrementAndGet() to before the print statement — ratchet freak
– ratchet freak, Commented May 23, 2014 at 10:48
You have an interesting problem here, and I can see you have taken some advice and applied it. I have a number of suggestions I would like to make about your code too, but, unfortunately, the best system to use in Code Review is to put your updated code in a new question. I am going to roll-back your edit so that the questions/answers all make sense in the future too. — rolfl
– rolfl, Commented May 23, 2014 at 11:26

ratchet freak · Accepted Answer · 2014-05-23 09:35:21Z

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<String, String> sequence = new HashMap<String, String>();

    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.

Thanks for your input, agree with spin lock and I should have used while instead of IF statement. However, doNotifyAll() even if it would have awoke up wrong thread, wouldn't the check about thread name and turn taken care of correct execution? — Manish Devraj
– Manish Devraj, Commented May 23, 2014 at 10:19
@ManishDevraj but in your code it won't get back into the if block to wake the other thread. For example the first thread just notifiedAll but gets stopped before the wait and and then the next thread does the same, now the third thread runs completely into the wait, now all threads are deadlocked in the waits. — ratchet freak
– ratchet freak, Commented May 23, 2014 at 10:25
got it. I need to make few tweaks related to getting exact number of digits printed. Updated original thread. Thanks for review :) — Manish Devraj
– Manish Devraj, Commented May 23, 2014 at 10:39
This code doesn't work due to use of int i = index.get(); . 1 is getting printed three times in the beginning. Another issues is integers - 11 , & 12 also get printed . That gets fixed when you do index.get() <= 8 instead of while (i <= 10) and don't use i. — Sabir Khan
– Sabir Khan, Commented Feb 1, 2017 at 4:37

Jamal · Accepted Answer · 2015-08-19 18:59:32Z

0

I found a small glitch over here: you should declare your variable i as below in class level, otherwise a few numbers might get printed twice/thrice or so.

volatile static int i;

//volatile variable will get reflected for each thread immediately

edited Aug 19, 2015 at 18:59

Jamal

35.2k13 gold badges134 silver badges238 bronze badges

answered Aug 19, 2015 at 10:11

Ajay Manjhi

1

\$\begingroup\$ It is not necessary to declare i at the class level. The reason behind this is that i is confined to the stack, so need to worry about the visibility of i. Why is it to be at the class level? How are you killing all the threads after completing the task? \$\endgroup\$

Damaji kalunge
– Damaji kalunge

2016-10-01 18:01:14 +00:00
Commented Oct 1, 2016 at 18:01

Add a comment |

Stack Exchange Network

Sequential Thread Execution using wait() and notifyAll()

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Sequential Thread Execution using wait() and notifyAll()

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions