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();
}
}
}
}
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 the design and general logic.
In your code, there are two classes that 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 separation of concerns is not dealt properly. 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.
In your code, both threads manage to print the right number only at the start (the System.out.println without any text) because:
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);
}
OddThread increments the value to 2, prints the notify statement, and finally notifies the other thread. Bear 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();
while condition is tested. The following if fails, as the number is now even, OddThread releases the lock and waits until is notified by EvenThread.try {
System.out.println("OddThread: Wait: Counter: " + counter);
wait();
} catch (InterruptedException e) {
System.out.println("Interrupted Exception!");
}
EvenThread can finally enter the synchronized statement and prints its value (2).if (counter % 2 == 0) {
System.out.println(counter);
}
EvenThread increments the value 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();
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!");
}
wait() invocation. Both of them resume when the number is their right "type", increment the 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 of 10 is reached. They both increment one more time after reaching the last while iteration, since they both resume from their last wait() call.Here is an implementation addressing the issues listed above.
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();
}
}