Java multithreading query

Question

Hi I am a newbie in concurrent programming domain, I was testing the below code and seems the while loop is not terminating for threads. Could someone help explain whats happening here.

public class PrimePrinter{
    public long counter = 0;
    public synchronized long getCounter() {
        return counter++;
    }
    public Thread makeThread() {
        Runnable rn = new Runnable() {          
            /* (non-Javadoc)
             * @see java.lang.Runnable#run()
             */
            @Override
            public void run() {
                while (counter < 100) {
                    try {
                        Thread.sleep(1000);
                        System.out.println(Thread.currentThread().getName() + " : " +getCounter());
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }                   
                }
            }
        };
        return new Thread(rn);
    }
    public static void main(String[] args) {
        int n = 10;
        Thread[] threads = new Thread[10];
        PrimePrinter pp = new PrimePrinter();
        for(int i = 1; i < n; i++) {            
            threads[i] = pp.makeThread();
            threads[i].start();
        }
    }
}

Last few lines of output

Thread-4 : 81
Thread-5 : 87
Thread-7 : 91
Thread-5 : 97
Thread-2 : 95
Thread-4 : 98
Thread-6 : 96
Thread-8 : 90
Thread-1 : 93
Thread-3 : 92
Thread-0 : 94
Thread-2 : 99
Thread-6 : 107
Thread-3 : 103
Thread-0 : 104
Thread-1 : 105
Thread-8 : 106
Thread-5 : 102
Thread-4 : 101
Thread-7 : 100

user_10830215 · Answer

This is not working for one reason that is crucial not to miss. Consider the way your code actually runs. It checks the counter value, then sleeps, THEN prints THEN increments.

You have 9 threads running at once. Then, 8 of these threads check the value, and at the time, the value is less than 100. So they pass the test and keep going. All 8 then sleep for 1000ms. Meanwhile, your other thread has just incremented the value... and 8 more are about to come out of their sleep.

So, if your value is 99 (which it inevitably will be), you'll now get a value of 100 from the thread that just incremented. Then, those 8 that already passed the test are also going to be incrementing, giving the counter a resulting value of 108.

However, the very last thread to run will show a value of 107 because you print THEN increment for all runs.

If you did this the other way around (like in the code below), you would not run into this problem.

The following code works correctly:

public class PrimePrinter {
    public long counter = 0;

    public synchronized long getCounter() {
        return counter;
    }

    public synchronized void incrementCounter() {
        counter++;
    }

    public Thread makeThread() {
        Runnable rn = new Runnable() {
            /*
             * (non-Javadoc)
             * 
             * @see java.lang.Runnable#run()
             */
            @Override
            public void run() {
                while (getCounter() < 100) {
                    try {
                        incrementCounter();
                        System.out.println(Thread.currentThread().getName()
                                + " : " + getCounter());
                        Thread.sleep(1000);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        };
        return new Thread(rn);
    }

    public static void main(String[] args) {
        int n = 10;
        Thread[] threads = new Thread[10];
        PrimePrinter pp = new PrimePrinter();
        for (int i = 1; i < n; i++) {
            threads[i] = pp.makeThread();
            threads[i].start();
        }
    }
}

user_7036419 · Answer

Each thread waits a second after having tested that counter < 100, during this second, other threads can increment the counter.

UPDATE: You could do something like this:

            while (true) {
                long current = getCounter();
                if (current >= 100) {
                    break;
                }
                try {
                    Thread.sleep(1000);
                    System.out.println(Thread.currentThread().getName() + " : " + current);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }                   
            }

Java multithreading query

2 Answers