CODEWITHSUNDEEP
javamultithreading
bbb
bbb

Reputation: 29

Java threadsafe counter out of bound

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

Upvotes: 3

Views: 105

Answers (2)

ernest_k
ernest_k

Reputation: 45329

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
    }
}

Upvotes: 0

Thomas Kl&#228;ger
Thomas Kl&#228;ger

Reputation: 21510

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.

Upvotes: 2

Related Questions