Java threadsafe counter out of bound

Question

I want to implement simple threadsafe counter. The numbers are in the right order so that part is ok, the only problem is the condition is not always met and sometimes the numbers go up to 51 or 52.

Should I use the tag synchronized also around the while loop?

I mean, I can double check and put a condition in the method printAndIncrement but that doesn't seem very elegant.

public class MyCounter implements Runnable {

    static int currentValue = 0;

    private static synchronized void printAndIncrement() {
        System.out.print(Thread.currentThread().getName() + ": " + currentValue + "\n");
        currentValue++;
    }

    @Override
    public void run() {
        while (currentValue <= 50) {
            printAndIncrement();
        }
    }

    public static void main(String[] args) {
        MyCounter counter = new MyCounter();
        Thread thread1 = new Thread(counter);
        Thread thread2 = new Thread(counter);
        Thread thread3 = new Thread(counter);
        thread1.start();
        thread2.start();
        thread3.start();
    }
}

Why not use AtomicInteger? https://docs.oracle.com/javase/10/docs/api/java/util/concurrent/atomic/AtomicInteger.html — Jeroen Steenbeeke, Apr 17 '18 at 20:03
I used it at first but while the incrementation would work okay, the AtomicInteger didnt guarantee that the printing would also be in order. — bbb, Apr 17 '18 at 20:07
Is your real code doing this incrementing and printing numbers in order? — ernest_k, Apr 17 '18 at 20:20
@Erik It seems to work as well, but at the same time I read comments like this: https://stackoverflow.com/a/25579778/9660678 — bbb, Apr 17 '18 at 20:38
@BetkaStrakošová, I would like to ask you about the goal you want to achieve. Do you want just to print the numbers in a proper order, using multiple threads *or* do you want to simulate some piece of work that can be split between threads and run in parallel? In other words, is output order important or is it just a helper for debugging purpose? — Krzysztof Kaszkowiak, Apr 17 '18 at 20:40
@BetkaStrakošová The code in the link you provided is not synchronised. There is unsynchronised code between the increment and the decrement where other threads can wreak havoc. In your example, if currentValue is not volatile, then the next thread Into the synch block might see a value that is old. Volatile ensures that all threads see the same value. Volatile means essentially atomic — Erik, Apr 18 '18 at 07:34

score 2 · Answer 1 · answered Apr 17 '18 at 20:16

The check currentValue <= 50 and the call to printAndIncrement must be in the same synchronized block. Otherwise this problem is going to happen.

Let currentValue be 50. All three threads can do the check that the current value is no more than 50 and then try to call printAndIncrement(); simultaneously.

Due to the synchronized void printAndIncrement() the threads will execute this method sequentially, but for the first thread the currentValue will be 50, for the second thread it will be 51 and for the third thread it will be 52.

score 0 · Answer 2 · answered Apr 17 '18 at 20:52

The problem is that your boundary check and the increment are not both synchronized, which defeats the point of synchronization altogether.

The best alternative I can suggest that allows both synchronizing read/update and allowing your loop to stop would be to make the incrementing method return a boolean:

/** Prints and increments, returning true if max value has not been reached */
private static synchronized boolean printAndIncrement() {
    if(currentValue < 51) {
        System.out.print(Thread.currentThread().getName() 
             + ": " + currentValue + "\n");
        currentValue++;

        return true;
    } else {
        return false;
    }
}

And change the run method to:

public void run() {
    while (printAndIncrement()) {
        //nothing needs to be done here
    }
}

Java threadsafe counter out of bound

2 Answers2