4
\$\begingroup\$

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<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();

    }
}
\$\endgroup\$
3
  • \$\begingroup\$ actually you could have moved the index.incrementAndGet() to before the print statement \$\endgroup\$ May 23, 2014 at 10:48
  • \$\begingroup\$ 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. \$\endgroup\$
    – rolfl
    May 23, 2014 at 11:26
  • \$\begingroup\$ For those who wants to take a stab at updated code link \$\endgroup\$ May 25, 2014 at 9:29

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ 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? \$\endgroup\$ May 23, 2014 at 10:19
  • \$\begingroup\$ @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. \$\endgroup\$ May 23, 2014 at 10:25
  • \$\begingroup\$ got it. I need to make few tweaks related to getting exact number of digits printed. Updated original thread. Thanks for review :) \$\endgroup\$ May 23, 2014 at 10:39
  • \$\begingroup\$ 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. \$\endgroup\$
    – Sabir Khan
    Feb 1, 2017 at 4:37
0
\$\begingroup\$

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 
\$\endgroup\$
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\$ Oct 1, 2016 at 18:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.