CODEWITHSUNDEEP
javamultithreadingsynchronizationthread-safetyatomic
Magnus
Magnus

Reputation: 18778

Thread safe vs. non thread safe implementations of a counter

This is a school assignment, but I really need some help with this one. I can't for the life of me figure out why the two different versions of nextValue() behave differently - one is thread safe and the other one is not. Can somebody give me some pointers in the right direction at least?

I've included both versions in the same class below, but obviously they're not both present in the code...

public class NumGenerator {

    static final int MIN_VALUE = -256;
    static final int MAX_VALUE = 255;
    static final int INITIAL_VALUE = MIN_VALUE -1;


    private final AtomicInteger counter = new AtomicInteger(INITIAL_VALUE);
    private final AtomicInteger resetCounter = new AtomicInteger(0); 

    private final Object lock = new Object();

    // Thread safe
    public int nextValue() {
        int next = counter.incrementAndGet();      
        if (next > MAX_VALUE) {                         
            synchronized (lock) {                       
                next = counter.incrementAndGet();   
                if (next> MAX_VALUE) {                  
                    counter.set(MIN_VALUE); 
                    resetCounter.incrementAndGet();
                    next = MIN_VALUE;
                }

            }
        }
        return next;   
    }

     // Non thread safe
     public int nextValue() {
        int next = counter.incrementAndGet();
        if (next > MAX_VALUE) {
            synchronized (lock) {
                int i = counter.get();
                if (i > MAX_VALUE) {
                    counter.set(INITIAL_VALUE);
                    resetCounter.incrementAndGet();
                }
                next = counter.incrementAndGet();
            }
        }
        return next;
    }
}

Upvotes: 0

Views: 877

Answers (3)

Dios-Malone
Dios-Malone

Reputation: 36

Both are Thread-Safe. Maybe you can paste out the code how you are testing them. The issue might be there.

Upvotes: 0

Kishore Bandi
Kishore Bandi

Reputation: 5721

Let's say values were : MIN_VALUE = -1, MAX_VALUE = 3, counter = 3.

Code 1:

synchronized (lock) {                       
                next = counter.incrementAndGet();   
                if (next> MAX_VALUE) {                  
                    counter.set(MIN_VALUE); 
                    resetCounter.incrementAndGet();
                    next = MIN_VALUE;
                }

            }
  1. It increments the value of counter and then uses it for comparison.
  2. So the value of next becomes 4. if(next > MAX_VALUE) becomes if(4>3) this would change the value of next to -1 and return it.

Code 2:

synchronized (lock) {
                int i = counter.get();
                if (i > MAX_VALUE) {
                    counter.set(INITIAL_VALUE);
                    resetCounter.incrementAndGet();
                }
                next = counter.incrementAndGet();
            }
  1. It assigns the value to counter then compares.
  2. The value of i would still be 3. if(i > MAX_VALUE) becomes if(3 > 3), which its not so returns 3 as the output.

incrementAndGet and get are different. So the code with same values is returning different output. one is incrementing the value first then checking the condition, the other is checking the values first and then performing an operation.

Even the code inside the if( variable > MAX_VALUE) would result in different output.

So this has nothing to do with Thread safety.

Upvotes: 1

Imtiaz Ali
Imtiaz Ali

Reputation: 362

Think Both methods are thread safe logically. because in both methods synchronized is used. and synchronized block is used for thread safety.

Upvotes: 0

Related Questions