Printing OddEven Number using 2 Threads in Java

I'm sure there are multiple answers to my question. But, I am learning the basic concepts of multi-threading and I came up with the code below.

There are two threads: one prints even numbers and the other prints odd numbers. For some reason, they both print their right number at first and then they "swap" role. Also, they seem to print more than just the first 10 numbers.

Why is it not giving the correct output?

package com.thread;

public class OddEventThread {

    public static void main(String[] args) {
        SharedResource obj = new SharedResource();

        OddThread oddThread = new OddThread(obj);
        EvenThread evenThread = new EvenThread(obj);

        System.out.println("Starting Odd/Even Thread");

        oddThread.start();
        evenThread.start();
    }

}

class OddThread extends Thread {

    SharedResource obj;

    public OddThread(SharedResource obj) {
        this.obj = obj;
    }

    @Override
    public void run() {
        System.out.println("OddThread");
        obj.printOdd();
    }
}

class EvenThread extends Thread {

    SharedResource obj;

    public EvenThread(SharedResource obj) {
        this.obj = obj;
    }

    @Override
    public void run() {
        System.out.println("EvenThread");
        obj.printEven();
    }
}

class SharedResource {

    private int N = 10;

    private int counter = 1;

    public void printOdd() {
        System.out.println("printOdd");
        synchronized (this) {
            System.out.println("OddThread: Counter: " + counter);
            while (counter <= N) {
                if (counter % 2 != 0) {
                    System.out.println(counter);
                } else {
                    try {
                        System.out.println("OddThread: Wait: Counter: " + counter);
                        wait();
                    } catch (InterruptedException e) {
                        System.out.println("Interrupted Exception!");
                    }
                }
                counter++;
                System.out.println("OddThread: Notify: Counter: " + counter);
                notify();
            }
        }
    }

    public void printEven() {
        System.out.println("printEven");
        synchronized (this) {
            System.out.println("EvenThread: Counter: " + counter);
            while (counter <= N) {
                if (counter % 2 == 0) {
                    System.out.println(counter);
                } else {
                    try {
                        System.out.println("EvenThread: Wait: Counter: " + counter);
                        wait();
                    } catch (InterruptedException e) {
                        System.out.println("Interrupted Exception!");
                    }
                }
                counter++;
                System.out.println("EvenThread: Notify: Counter: " + counter);
                notify();
            }
        }
    }
}

Output:

Starting Odd/Even Thread
OddThread
printOdd
EvenThread
printEven
OddThread: Counter: 1
1
OddThread: Notify: Counter: 2
OddThread: Wait: Counter: 2
EvenThread: Counter: 2
2
EvenThread: Notify: Counter: 3
EvenThread: Wait: Counter: 3
OddThread: Notify: Counter: 4
OddThread: Wait: Counter: 4
EvenThread: Notify: Counter: 5
EvenThread: Wait: Counter: 5
OddThread: Notify: Counter: 6
OddThread: Wait: Counter: 6
EvenThread: Notify: Counter: 7
EvenThread: Wait: Counter: 7
OddThread: Notify: Counter: 8
OddThread: Wait: Counter: 8
EvenThread: Notify: Counter: 9
EvenThread: Wait: Counter: 9
OddThread: Notify: Counter: 10
OddThread: Wait: Counter: 10
EvenThread: Notify: Counter: 11
OddThread: Notify: Counter: 12

Here is my thought process of coming up with this solution:

we have 2 threads which print numbers from 1 to 10. Both threads should share an object, hence I came up with one sharedObj. So, since the same object is shared between 2 threads, then the synchronized block should work in properly updating the value.

There are some issues in your implementation regarding its design, generalization and general logic.

You're declaring two classes which basically do the exact same thing: printing numbers. The only thing that differs is the condition: whether the printed numbers must be odd or even. You could already achieve this with one Thread class, where the only thing to parameterize is the printing condition. The printOdd() and printEven() methods are almost a copy/paste.

Furthermore, the class responsibilities are not well dealt. Your SharedObject class is basically a counter, which not only keeps track and increments the counting value, but it has also to deal with both threads' logic, which is something that should not fall on it. Its sole goal should be to increment a shared value in a consistent way with parallel execution. You should redirect that printing logic within your Thread, since, as you can see, their code basically consists in making a single call.

Answer

The logic within your printOdd and printEven() methods has some loopholes. Both threads manage to print their corresponding number type only once at the start (the System.out.println without any text). This is because:

1. One of the threads acquires the SharedObject's monitor, let's say OddThread, which means that EvenThread awaits the lock release in order to enter the synchronized block. At this point, OddThread prints its corresponding number (1):

if (counter % 2 != 0) {
    System.out.println(counter);
}

2. Then, OddThread increments the value to 2, prints the notify statement and finally notifies the other thread. Bbear in mind that while the thread is doing all of this, it's still owning the SharedObject's monitor.

counter++;
System.out.println("OddThread: Notify: Counter: " + counter);
notify();

3. Then, the loop ends and the while condition is tested. The following if fails as the number is now even and OddThread releases the lock and waits until it gets notified by EvenThread.

try {
    System.out.println("OddThread: Wait: Counter: " + counter);
    wait();
} catch (InterruptedException e) {
    System.out.println("Interrupted Exception!");
}

4. Then, EvenThread can finally enter the synchronized statement and prints its value, which is 2.

if (counter % 2 == 0) {
    System.out.println(counter);
}

5. Then, it increments the value from 2 to 3, prints the notify statement with value 3 (so it's printing twice and also the wrong number) and finally notifies the other thread.

counter++;
System.out.println("EvenThread: Notify: Counter: " + counter);
notify();

6. Then, EvenThread tests the while condition, fails the if statement, prints the waiting statement and then awaits for OddThread to awake it.

try {
    System.out.println("EvenThread: Wait: Counter: " + counter);
    wait();
} catch (InterruptedException e) {
    System.out.println("Interrupted Exception!");
}

7. From now on, each thread will continue from their last wait() invocation. Both of them resume when the number is their right "type", increment its value (making it their opposing type), and then print the notify statement. This, not only explains why each thread prints their opposing number type, but it also explains why they keep printing even after the max value 10 has been reached. They both increment one more time after reaching the last while iteration, since they both resume from their last wait() call.

Solution

Here is an implementation with all the design, generalization and logic loopholes fixed.

class Main {

    public static void main(String[] args) {
        SharedCounter counter = new SharedCounter();

        ThreadPrintingNums oddThread = new ThreadPrintingNums("OddThread", counter, false, 10);
        ThreadPrintingNums evenThread = new ThreadPrintingNums("EvenThread",counter, true, 10);

        System.out.println("Starting Threads");

        oddThread.start();
        evenThread.start();
    }
}

class ThreadPrintingNums extends Thread {

    private SharedCounter counter;
    private boolean flagPrintEven;
    private int max;

    public ThreadPrintingNums(String threadName, SharedCounter obj, boolean flagPrintEven, int max) {
        setName(threadName);
        this.counter = obj;
        this.flagPrintEven = flagPrintEven;
        this.max = max;
    }

    @Override
    public void run() {
        while (counter.getCounter() <= max) {
            if (counter.getCounter() % 2 == (flagPrintEven ? 0 : 1)) {
                System.out.printf("%s => %d%n", getName(), counter.getCounter());
                counter.incCounter();
            } else {
                try {
                    synchronized (counter) {
                        counter.wait();
                    }
                } catch (InterruptedException e) {
                    System.out.printf("%s interrupted exception", getName());
                    System.exit(-1);
                }
            }
        }
    }
}

class SharedCounter {

    private int counter;

    public SharedCounter() {
        this.counter = 1;
    }

    public synchronized int getCounter() {
        return counter;
    }

    public synchronized void incCounter() {
        counter++;
        notify();
    }
}