wait()/notify() not working properly

Question

I have a ConsumerProducer object on which I want to acquire lock from two different threads. The class is as below:

public class ConsumerProducer {

    public String stringPool = null;

    public void put(String s){
        stringPool = s;
    }

    public String get(){
        String ret = stringPool;
        stringPool = null;
        return ret;
    }

}

The thread impl class is as below:

public class WaitNotifyTest implements Runnable {

    private String threadType;
    public ConsumerProducer cp;
    public static volatile int i = 1;

    public WaitNotifyTest(String threadType, ConsumerProducer cp) {
        this.threadType = threadType;
        this.cp = cp;
    }

    public static void main(String[] args) throws InterruptedException {

        ConsumerProducer cp = new ConsumerProducer();
        WaitNotifyTest test1 = new WaitNotifyTest("Consumer", cp);
        WaitNotifyTest test2 = new WaitNotifyTest("Producer", cp);

        Thread t1 = new Thread(test1);
        Thread t2 = new Thread(test2);

        t1.start();
        t2.start();
        t1.join();
        t2.join();
    }

    @Override
    public void run() {
        while (true) {

            if (threadType.equalsIgnoreCase("Consumer")) {
                synchronized (cp) {
                    try {
                        if (null != cp.get()) {
                            cp.wait();
                        }
                        consume();
                        System.out.println("notify from Consumer");
                        cp.notify();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

            } else {
                synchronized (cp) {
                    try {
                        if (null == cp.get()) {
                            cp.wait();
                        }
                        produce();
                        System.out.println("notify from Producer");
                        cp.notify();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
            if (i == 5) {
                break;
            }
            i++;
        }
    }

    public void consume() {
        System.out.println("Putting: Counter" + i);
        cp.put("Counter" + i);
    }

    public void produce() {
        System.out.println("getting: " + cp.get());
    }

}

But when I run the code it is facing some kind of deadlock and it is stuck printing like

Putting: Counter3
notify from Consumer

Something is going terribly wrong but I am not able to identify. Please help.

I usually try to avoid wait/notify because they're harder to reason about than things like CountDownLatch and CyclicBarrier. — David Ehrmann, Jul 24 '16 at 15:17

Eugene · Accepted Answer · 2016-07-24T15:15:57.613

Your consumer is doing producer's job and producer is doing consumer's job. Exchange their responsibility and modify the condition to wait. Please refer to the code below.

Consumer will wait when there is nothing to get and he will release the lock of cp. So that producer has chance to go into the synchronized block.
Producer only produces when there is nothing or he will wait. After that, he will release the lock of cp. So that consumer has chance to go into the synchronized block.
Consumer is who get things away.
Producer is who put things to table.
According to your comment. You want to put Counter from 1 to 5, so you should add i++ only in Producer thread. How can you control its increase in both threads?

You don't judge whether it's consumer or producer calling the get() from cp object but assign null to stringPool. It's obvious wrong and will make consumer get null from public space. I add a new method clearString() which will set public space to null only when consumer has comsumed the product.

public class WaitNotifyTest implements Runnable {

private String threadType;
public ConsumerProducer cp;
public static volatile int i = 0;

public WaitNotifyTest(String threadType, ConsumerProducer cp) {
    this.threadType = threadType;
    this.cp = cp;
}

public static void main(String[] args) throws InterruptedException {

    ConsumerProducer cp = new ConsumerProducer();
    WaitNotifyTest test1 = new WaitNotifyTest("Consumer", cp);
    WaitNotifyTest test2 = new WaitNotifyTest("Producer", cp);

    Thread t1 = new Thread(test1);
    Thread t2 = new Thread(test2);

    t1.start();
    t2.start();
    t1.join();
    t2.join();
}

@Override
public void run() {
    while (true) {
        if (threadType.equalsIgnoreCase("Consumer")) {
            synchronized (cp) {
                try {
                    /*
                     * Consumer will wait when there is nothing to get and he will release the lock of cp.
                     * So that producer has change to go into the synchronized block.
                     */

                    if (null == cp.get()) {
                        cp.wait();
                    }
                    consume();
                    System.out.println("notify from Consumer");
                    cp.notify();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }

        } else {
            synchronized (cp) {
                try {
                    /*
                     * Producer only produce when there is nothing or he will wait. At the same time, he will release the lock of cp.
                     * So that consumer has chance to go into the synchronized block.
                     */
                    if (null != cp.get()) {
                        cp.wait();
                    }
                    i++;
                    produce();
                    System.out.println("notify from Producer");
                    cp.notify();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
        if (i == 5) {
            break;
        }

    }
}

public void consume() {
    System.out.println("getting: " + cp.get());
    cp.clearString();
}

public void produce() {
    System.out.println("Putting: Counter" + i);
    cp.put("Counter" + i);
}}

Also see the ConsumerProducer class.

public class ConsumerProducer {
        public String stringPool = null;

        public void put(String s){
            stringPool = s;
        }

        public String get(){
            return stringPool;
        }

        public void clearString(){
            stringPool = null;
        }
}

Thanks @Gearon. Deadlock is gone. However the output is not as expected. Output: `Putting: Counter1 notify from Producer getting: null notify from Consumer Putting: Counter3 notify from Producer getting: Counter3 notify from Consumer Putting: Counter5 notify from Producer getting: Counter5 notify from Consumer` Inconsistent though. — Anirban B, Jul 24 '16 at 13:51
Expected Output: `Putting: Counter1 notify from Producer getting: Counter1 notify from Consumer Putting: Counter2 notify from Producer getting: Counter2 notify from Consumer Putting: Counter3 notify from Producer getting: Counter3 notify from Consumer Putting: Counter4 notify from Producer getting: Counter4 notify from Consumer Putting: Counter5 notify from Producer getting: Counter5 notify from Consumer` — Anirban B, Jul 24 '16 at 13:51
Thanks @Gearon I got the point 5. However have confusion with point# 6. Instead of separate method I have declared stringPool as `public volatile String stringPool = null;`. Still I am not sure how is it getting null. Could you please elaborate what did you ean by "It's obvious wrong and will make consumer get null from public space"? — Anirban B, Jul 24 '16 at 17:20
@AnirbanB. if (null != cp.get()) { cp.wait(); } When producer call cp.get() to check whether the public space is empty or not, he will empty the public space but it's not his responsibility, it's consumer's responsibility. That means, producer will wait when he finds the public space is not empty, but he will empty the public space firstly, when consumer gets the lock, he can get nothing from public space. — Eugene, Jul 24 '16 at 23:36

score 0 · Answer 2 · answered Jul 25 '16 at 06:25

Updated code is here: ConsumerProducer.java:
public class ConsumerProducer {

    public volatile String stringPool = null;

    public void put(String s){
        this.stringPool = s;
    }

    public String get(){
        String ret = this.stringPool;
        //this.stringPool = null;
        return ret;
    }
    //added
    public void clearString(){
        this.stringPool = null;
    }

}

WaitNotifyTest.java public class WaitNotifyTest implements Runnable {

    private String threadType;
    public ConsumerProducer cp;
    public static volatile int i = 0;

    public WaitNotifyTest(String threadType, ConsumerProducer cp) {
        this.threadType = threadType;
        this.cp = cp;
    }

    public static void main(String[] args) throws InterruptedException {

        ConsumerProducer cp = new ConsumerProducer();
        WaitNotifyTest test1 = new WaitNotifyTest("Consumer", cp);
        WaitNotifyTest test2 = new WaitNotifyTest("Producer", cp);

        Thread t1 = new Thread(test1);
        Thread t2 = new Thread(test2);

        t1.start();
        t2.start();
        t1.join();
        t2.join();
    }

    @Override
    public void run() {
        while (true) {

            if (threadType.equalsIgnoreCase("Consumer")) {
                synchronized (cp) {
                    try {
                        if (null == cp.get()) {
                            cp.wait();
                        }
                        consume();
                        System.out.println("notify from Consumer");
                        cp.notify();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

            } else {
                synchronized (cp) {
                    try {
                        if (null != cp.get()) {
                            cp.wait();
                        }
                        i++;
                        produce();
                        System.out.println("notify from Producer");
                        cp.notify();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
            if (i == 5) {
                break;
            }

        }
    }

    public void produce() {
        System.out.println("Putting: Counter" + i);
        cp.put("Counter" + i);
    }

    public void consume() {
        System.out.println("getting: " + cp.get());
        cp.clearString();
    }

}

wait()/notify() not working properly

2 Answers2