Print Even Odd number using two threads

Question

I was trying to print even and odd numbers by two threads repetitively using wait and notify. However, I have gone through all the implementations given in website. Though as a first time Multi threading developer I was trying to do it my self, but I could not get the desired result. Here I am pasting my code below: Could you please review and revert back with the corrections and explanations where I made the mistake.

package com.test.printEvenOdd;

public class PrintOddEvenNumbers {

    public static void main(String[] args){

        String s = new String("");

        EvenThread t1= new EvenThread(s);
        OddThread t2= new OddThread(s);
        Thread th1 = new Thread(t1);
        Thread th2 = new Thread(t2);
        th1.start();
        th2.start();
    }

}

class EvenThread implements Runnable{
    String s;
    EvenThread(String s){
        this.s= s;
    }

    @Override
    public void run() {
        synchronized(s){
            for(int i=1;i<=10;i++){

                if(i%2==0){
                    try {
                        Thread.sleep(50);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                    System.out.println(i);
                    s.notify();
                }
                try {
                    s.wait();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }

        }
    }

}

class OddThread implements Runnable{

    String s;
    OddThread(String s){
        this.s= s;
    }

    @Override
    public void run() {
        synchronized(s){
            for(int i=1;i<=10;i++){
                try {
                    Thread.sleep(50);
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
                if(i%2==1){
                    System.out.println(i);
                    s.notify();
                }
                try {
                    s.wait();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }
    }

}

Answer 1

To understand what's happening a bit better, let's go through the steps happening in each Thread.

public class PrintOddEvenNumbers {

    public static void main(String[] args){

        String s = new String("");

        EvenThread t1= new EvenThread(s);
        OddThread t2= new OddThread(s);
        Thread th1 = new Thread(t1);
        Thread th2 = new Thread(t2);
        th1.start();
        th2.start();
    }

}

class EvenThread implements Runnable{
    String s;
    EvenThread(String s){
        this.s= s;
    }

    @Override
    public void run() {
        synchronized(s){
            for(int i=1;i<=10;i++){
                System.out.println("EvenThread i: " + i);

                if(i%2==0){
                    try {
                        Thread.sleep(50);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                    System.out.println(i);
                    System.out.println("EvenThread notify");
                    s.notify();
                }
                try {
                    System.out.println("EvenThread waiting..");
                    s.wait();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }

        }
    }
}

class OddThread implements Runnable{

    String s;
    OddThread(String s){
        this.s= s;
    }

    @Override
    public void run() {
        synchronized(s){
            for(int i=1;i<=10;i++){
                System.out.println("OddThread i: " + i);

                try {
                    Thread.sleep(50);
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
                if(i%2==1){
                    System.out.println(i);
                    System.out.println("OddThread notify");
                    s.notify();
                }
                try {
                    System.out.println("OddThread waiting..");
                    s.wait();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }
    }
}

This will print:

EvenThread i: 1
EvenThread waiting..
OddThread i: 1
1
OddThread notify
OddThread waiting..
EvenThread i: 2
2
EvenThread notify
EvenThread waiting..
OddThread i: 2
OddThread waiting..

A simple explanation:

• When OddThread reaches of i of 2, it waits for s to be released.
• When EvenThread reaches i of 2, it also waits for s to be released.

You now have both threads waiting to be woken up (deadlock).

This happens because of the conditions that need to be met in order to wake the other waiting thread up using notify i.e. i%2==1 and i%2=0.

This isn't the only problem however, there are also some fundamental issues.

• The usage of Threads in this particular case if it were in production is incorrect as you're trying to do sequential work anyway, so the overhead of creating Threads for each task adds unnecessary overhead.
• There is no resource being shared, making synchornize redundant.
• You expect that one Thread will get a hold of a lock before the other, this isn't how Thread's work - it can be either one that gets a hold of the lock first.

Answer 2

There are a number of issues with your initial code. See GhostCat's answer for explanations of them. In general, this sort of computation isn't great for multi threading since you are (apparently) wanting the numbers printed sequentially. But, given that desire and wanting to use 2 threads interleaving to do that, you could do it as follows. Note that there are still some problems with this solution. The thread depends on a different thread having executed to be able to reach it's own end condition which means that if you only created one for odd (or even) numbers, you'd go into an infinite loop.

import java.util.Objects;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.function.IntPredicate;

public class Foo {

    public static void main(String[] args) {
        // an executor service will handle the thread pool and scheduling
        ExecutorService pool = Executors.newFixedThreadPool(2);
        pool.submit(new NumberPrintAndIncrement(i -> i % 2 != 0));
        pool.submit(new NumberPrintAndIncrement(i -> i % 2 == 0));
        // you want to shut down the pool when the threads are done
        pool.shutdown();
    }
}

final class NumberPrintAndIncrement implements Runnable {

    // Need a shared lock for accessing and updating the current number
    private static final Object LOCK = new Object();
    // The number is shared between threads so it needs to be volatile
    private static volatile int number = 1;

    // Instance variable for letting a particular runnable know if it should
    // print the number in it's current state
    private final IntPredicate predicate;

    NumberPrintAndIncrement(IntPredicate predicate) {
        this.predicate = Objects.requireNonNull(predicate);
    }

    @Override
    public void run() {
        while (number < 10) {
            // this could run at any point and any number of times, but
            // that doesn't matter since it is just doing a quick check and
            // a possible update. If the number doesn't satisfy the predicate,
            // this will just be a no-op. Having a predicate means
            // you don't have to rely on wait and notify to try and
            // achieve interleaving the number output properly which
            // is good due to the liveness problem Rajesh mentioned.
            synchronized (LOCK) {
                if (predicate.test(number)) {
                    System.out.println(number);
                    number++;
                }
            }
        }
    }
}

Answer 3

Your problem is that you locking is too conservative/restrictive:

You put the lock around the whole loop; for both threads.

So, one thread gets into its loop; but quickly it can't progress. Because it would need that other thread to progress. But the second thread can't even start - because it can enter its loop at all!

In other words: in order to make progress; both threads need to be able to enter their respective loops; and make enough progress so that the other thread can do its next step.

Thats like building a room that only two person can exit together; but then you allow only one person to enter that room.

Welcome to multi-threaded programming; you just created your first dead-lock.

And for the record: when re-arranging the locks; make sure that you get the signaling right; so that wait/notify can work as supposed.

Finally: if you look carefully at your code; you will find that you duplicated a lot of code. That is always a bad idea. Instead: try to figure which parts are really different; and anything else ... should exist exactly once in your source code. So, as another exercise: when you re-arranged your code so that it does what it is supposed to do - try if you can refactor it, so that the amount of code duplication is minimized. I guarantee you, that will be an exercise worth spending your time on!

Answer 4

You should move the "wait()" inside the "if" block. Else thread will go in to wait without notifying the other waiting thread and both of them will be waiting.

        if(i%2==0){
            synchronized(s){
                System.out.println(i);
                try {
                    s.notify();
                    s.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }

There are issues with the code. There is no need for sleep. As mentioned in previous response, you are synchronizing too eagerly which is unnecessary. There is no guarantee whether even thread will start first or odd thread will start first. It depends on whichever thread manages to acquire lock first. In the end, one thread will be waiting forever as the other thread would have come already come out and no one will notify after that. And any wait() code should handle spurious wakeup explained here