Printing Odd Even number sequentially using two threads

Question

I am not able to understand why below program is not working in odd even scenario.

Output I am getting is In Odd Thread 1

The program execution if I understand-

Goes to t1 thread. Condition not satisfied. Calls notifyAll which goes unnoticed.Goes to t2 thread, which prints odd number increments and calls wait(). T2 should begin operation in this case right??

public class OddEvenTest {

    static int max = 20;
    static int count = 1;
    static Object lock = new Object();

    public static void main(String[] args) {

        Thread t1 = new Thread(new Runnable() {

            @Override
            public void run() {
                synchronized (lock) {
                    while(count % 2  == 0 && count < max) {
                        System.out.println("In even Thread " +count);
                        count++;
                        try {
                            lock.wait();
                        } catch (InterruptedException e) {
                            // TODO Auto-generated catch block
                            e.printStackTrace();
                        }
                    }

                    lock.notifyAll();
                }
            }
        });

        Thread t2 = new Thread(new Runnable() {

                    @Override
                    public void run() {
                        synchronized (lock) {
                            while(count % 2  != 0 && count < max) {
                                System.out.println("In Odd Thread " +count);
                                count++;
                                try {
                                    lock.wait();
                                } catch (InterruptedException e) {
                                    // TODO Auto-generated catch block
                                    e.printStackTrace();
                                }               
                            }
                            lock.notifyAll();
                        }

                    }
                });

        t1.start();
        t2.start();

    }

}

Answer 1

There are many problems i can think of

1. notifyAll has been written outside the loop so think about a scenario where odd thread is getting executed and printed the value and it is about to call wait but even thread has called notify before odd thread call wait so odd thread will always be in waiting state. So you should keep notifyAll inside loop.
   This is something you have experienced in your execution.

2. while loop should keep executing till count is less than max and the even or condition should be applied inside while loop.

3. wait should go in else condition

Refactored Code

public class OddEvenTest {

  static int max = 20;
  static int count = 1;
  static Object lock = new Object();

  public static void main(String[] args) {

    Thread t1 = new Thread(new Runnable() {

      @Override
      public void run() {
        synchronized (lock) {
          while(count < max) {
            if(count % 2  == 0 ) {
              System.out.println("In even Thread " + count);
              count++;
            }else{
              try {
                lock.wait();
              } catch (InterruptedException e) {
                System.out.println(e.getMessage());
                e.printStackTrace();
              }
            }
            lock.notifyAll();
          }
        }
      }
    });

    Thread t2 = new Thread(new Runnable() {

      @Override
      public void run() {
        synchronized (lock) {
          while(count < max) {
            if(count % 2  != 0) {
              System.out.println("In Odd Thread " + count);
              count++;
            }else{
              try {
                lock.wait();
              } catch (InterruptedException e) {
                e.printStackTrace();
                System.out.println(e.getMessage());
              }
            }
            lock.notifyAll();
          }
        }

      }
    });

    t1.start();
    t2.start();

  }
}

Answer 2

You don't want to blindly wait() in a loop synchronized on your lock. Instead, check your pre-conditions for waiting, like

Thread t1 = new Thread(new Runnable() {
    @Override
    public void run() {
        while (count < max) {
            synchronized (lock) {
                if (count % 2 == 0) {
                    System.out.println("In Even Thread " + count);
                    count++;
                    lock.notifyAll();
                } else {
                    try {
                        lock.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }
});

Thread t2 = new Thread(new Runnable() {
    @Override
    public void run() {
        while (count < max) {
            synchronized (lock) {
                if (count % 2 != 0) {
                    System.out.println("In Odd Thread " + count);
                    count++;
                    lock.notifyAll();
                } else {
                    try {
                        lock.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }
});

In Java 8+, you can simplify your anonymous classes with lambdas like

Thread t1 = new Thread(() -> {
    while (count < max) {
        synchronized (lock) {
            if (count % 2 == 0) {
                System.out.println("In Even Thread " + count);
                count++;
                lock.notifyAll();
            } else {
                try {
                    lock.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
});

Thread t2 = new Thread(() -> {
    while (count < max) {
        synchronized (lock) {
            if (count % 2 != 0) {
                System.out.println("In Odd Thread " + count);
                count++;
                lock.notifyAll();
            } else {
                try {
                    lock.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
});