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

"seems the while loop is not terminating for threads" - why do you think so? doesn't your output indicate that while loop is terminating for threads? — eis
– eis, Commented Jan 3, 2019 at 10:32
it's because your threads are all executing the while loop, but only 1 is incrementing the counter. The lines are printed before the terminating condition 'arrives' at all threads — Tim
– Tim, Commented Jan 3, 2019 at 10:33
It's probably because while (counter < 100) { is not synchronized so all threads compare counter to 100, it's 99 and they all get a chance to run one more time. Try using an AtomicLong instead of the long variable or calling getCounter() in the loop check — Vlad Topala
– Vlad Topala, Commented Jan 3, 2019 at 10:34

Sal · Accepted Answer · 2019-01-03 22:59:06Z

4

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();
        }
    }
}

edited Jan 3, 2019 at 22:59

answered Jan 3, 2019 at 10:49

Sal

3081 silver badge11 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

sathish j Over a year ago

Hi Saleh, thanks for your insight. Now I understand the problem better. But I think incrementation and print should be atomize into a single synchronized block. Snce I am getting below output when executed your code. Thread-4 : 81 Thread-6 : 81 Thread-1 : 81 Thread-6 : 90 Thread-0 : 90 Thread-5 : 90 Thread-7 : 90 Thread-1 : 90 Thread-4 : 90 Thread-8 : 90 Thread-2 : 90 Thread-3 : 90 Thread-5 : 99 Thread-2 : 99 Thread-3 : 99 Thread-7 : 99 Thread-1 : 99 Thread-0 : 99 Thread-4 : 99 Thread-8 : 99 Thread-6 : 99 Thread-5 : 100

Maurice Perry Over a year ago

It still can print values higher than 100.

Sal Over a year ago

Yes, @MauricePerry it could, though it is extremely, extremely unlikely because of the speed at which the counter is checked and incremented. Also, sathish, I was not aware that you wanted this. I've modified the code accordingly.

Maurice Perry · Accepted Answer · 2019-01-03 10:41:48Z

2

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();
                }                   
            }

edited Jan 3, 2019 at 10:41

answered Jan 3, 2019 at 10:35

Maurice Perry

9,6802 gold badges15 silver badges28 bronze badges

1 Comment

sathish j Over a year ago

Thanks Maurice for your insight and your code works perfectly.

Collectives™ on Stack Overflow

Java multithreading query

2 Answers 2

3 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related