Wait and Notify not behaving as expected , thread is getting hanged

Question

Below is the code sample: Thread is getting hanged after consuming 5 or 6 values I do not know where i am missing anything.And one more doubt that i Had was regarding calling the constructor of the MyIncrementor class. Initially I was trying to call the get and set in Producer and Consumer Class by creating new object of the MyIncrementor class, it was not working too

/**
 * 
 */
package multithreadingDemo;

/**
 * @author Aquib
 *
 */
public class ThreadCommDemo {

    /**
     * @param args
     * @throws InterruptedException 
     */
    public static void main(String[] args) throws InterruptedException {
        // TODO Auto-generated method stub
        MyIncrementor mi=new MyIncrementor();
        Producer1 p=new Producer1(mi);
        Consumerrs c=new Consumerrs(mi);
        Thread t1=new Thread(p);
        Thread t2=new Thread(c);
        t1.start();
        t2.start();


    }

}



class MyIncrementor {
    int myCount;
    boolean valueSet;

    /**
     * @return the myCount
     */
    public synchronized int getMyCount() {
        if (!valueSet) {
            try {
                wait();
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
        System.out.println("get" + myCount);
        valueSet = true;
        notifyAll();
        return myCount;
    }

    /**
     * @param myCount
     *            the myCount to set
     */
    public synchronized void setMyCount(int myCount) {
        if (valueSet) {
            try {
                wait();
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
        System.out.println("Set" + myCount);
        this.myCount = myCount;
        valueSet = false;
        notifyAll();
    }

}

class Producer1 implements Runnable {
    MyIncrementor mi;

    public Producer1(MyIncrementor mi) {
        // TODO Auto-generated constructor stub
        this.mi=mi;
    }

    public void run() {

        for (int i = 1; i < 10; i++) {
            mi.setMyCount(i);
            System.out.println("Produced" + mi.myCount);
             try 
             {
                   Thread.currentThread().sleep((int)(Math.random() * 100));
             } 
             catch (InterruptedException ie) 
             {
                   ie.printStackTrace();
             }
        }
    }
}

class Consumerrs implements Runnable {
    MyIncrementor mi;

    public Consumerrs(MyIncrementor mi) {
        // TODO Auto-generated constructor stub
        this.mi=mi;
    }

    public void run() {

        for (int i = 1; i < 10; i++) {
            int val = mi.getMyCount();
            System.out.println("Consumed" + val);
        }

    }

}

Answer 1

You just have logical mistakes in your MyIncrementor class, you are setting valueSet incorrectly, so you have to either change your conditions or set the flag vice versa in getMyCount and setMyCount methods.

So, here is the corrected version of MyIncrementor:

class MyIncrementor {
  int myCount;
  boolean valueSet = false; //line changed - just to show that by default it is initialized to false

  /**
   * @return the myCount
   */
  public synchronized int getMyCount() {
    while (!valueSet) { //corrected as advised in comments, see @Tom Hawtin - tackline answer for details
      try {
        wait();
      } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
      }
    }
    System.out.println("get" + myCount);
    valueSet = false; //line changed - after getting the value set flag to false
    notifyAll();
    return myCount;
  }

  /**
   * @param myCount
   *            the myCount to set
   */
  public synchronized void setMyCount(int myCount) {
    while (valueSet) { //corrected as advised in comments, see @Tom Hawtin - tackline answer for details
      try {
        wait();
      } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
      }
    }
    System.out.println("Set" + myCount);
    this.myCount = myCount;
    valueSet = true; //line changed - after setting the value set flag to true
    notifyAll();
  }

}

Answer 2

The first thing that stands out is the lack of a while.

    if (valueSet) { // <-- if is wrong
        try {
            wait();

wait should be in a while. This ensures that the condition is met, rather than waking up for some other reason or no reason at all.

    while (valueSet) {
        try {
            wait();

Presumably the problem is that you have your flag the wrong way around.

    if (!valueSet) {
        // ...
    }
    // ...
    valueSet = true;

Presumably the if (should be a while) is intended that valueSet is true, but then you overwrite true with true rather than modifying the variable.

Also of not, Thread.sleep is a static method. It is incredibly misleading (though a common mistake) to call it on an instance.

Thread.currentThread().sleep(/* ... */); // misleading

should be

Thread.sleep(/* ... */);