CODEWITHSUNDEEP
javaconcurrency
dowonderatwill
dowonderatwill

Reputation: 139

java AtomicInteger decrementAndGet does not work as expected

The AtomicInteger's decrementAndGet does not seems working correctly in multi-threaded operation. Though the incrementAndGet works correctly. I run this program from the Executors and in 10 threads. Each thread is decrementing the value 5 times in the loop so 10 threads will reduce its value to -50. But I do not get the value -50. Same thing if I do for calling incrementAndGet then, it does increase to 50. 

public class RunCountAtomic {

public static  AtomicInteger aValue = new AtomicInteger(0);

public void runLoop() {
    for(int i=0;i<5;i++) {
//      aValue.getAndIncrement();
            doSomeWork();
            aValue.decrementAndGet();
        }
    System.out.println(" Expected value is - (5 x 10 thread = -50) , actual is: " +aValue.get());
    }
    // Some dummy work!     
    public void doSomeWork() { try {Thread.sleep(100);  } catch (InterruptedException e) {}  } 
}

The result shows that incrementAndGet does work correctly but not the decrementAndGet! Does anyone of you also have such observation? Or I might be missing something in the code.

Below is the test code which calls the method of above class:

[Updated as per reply comment] Update my TestMainRL code to use executor service to simplify it.

public static void main(String[] args) {
    int noOfThreads         = 10;
    ExecutorService e = Executors.newFixedThreadPool(noOfThreads); 

    List<Future<Boolean>>   fblist = new ArrayList<Future<Boolean>>(noOfThreads);  

    System.out.println("Starting");     
    // Run the async task in multiple threads.
    for (int i = 0; i<noOfThreads; i++) {
        TestMainRL t = new TestMainRL();
        fblist.add(t.asyncTask ( e));
    }

    //Just to wait all thread to complete.
    //try {for(Future<Boolean> fb: fblist) {fb.get();}} catch (Exception e1) {e1.printStackTrace();}
    e.shutdown();
    System.out.println("Completed!");

}

BTW, below is the result to show that last thread is not -50:

Starting
Completed!
Thread[pool-1-thread-1,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -42
Thread[pool-1-thread-2,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -42
Thread[pool-1-thread-6,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -46
Thread[pool-1-thread-7,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -50
Thread[pool-1-thread-5,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -49
Thread[pool-1-thread-3,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -49
Thread[pool-1-thread-4,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -49
Thread[pool-1-thread-10,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -46
Thread[pool-1-thread-8,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -46
Thread[pool-1-thread-9,5,main] Expected value is - (5 x 10 thread = -50) , actual is: -46

After getting the reply from @acm, I realized to achieve the goal that only one of the thread should print -50 and it would be possible with below code, which does not use ReentrantLock with below code of RunCountAtomic:

public class RunCountAtomic {
    public static  AtomicInteger aValue = new AtomicInteger(0);
    public void runLoop() {
        int x = 0;
        for(int i=0;i<5;i++) {
           doSomeWork();
           x = aValue.decrementAndGet(); // decrement and store it in local x.
       }
       // Here at the time of this query post, I was printing aValue.get() instead of using stored x,
       // which will ensure that only one thread will have -50.
       System.out.println(Thread.currentThread()+" Value in thread " +x);
    }
    public void doSomeWork() { 
        try {
             Thread.sleep(100);
        } catch (InterruptedException e) {} 
    } // Some dummy work!
}

The thread getting -50 is the last thread, which did the last decrement. But while doing system.out it may not be printing at the end.

Upvotes: 0

Views: 620

Answers (1)

acm
acm

Reputation: 2120

What are you expecting exactly? All threads to return -50, last thread to return -50? Non of that is guaranteed in the code you have provided.

AtomicInteger is behaving as expected. The guarantee you do have is that at least one of the threads will return -50, and that's happening right?

The key point is System.out.println -> PrintStream#println:

public void println(String x) {
    synchronized (this) {
        print(x);
        newLine();
    }
}

So all your thread are calling this but there is not fairness in this type of lock. So even though the last thread to arrive at this lock was the one with value -50 it might not be the first one to get printed, so you have the guarantee that -50 is printed but at some point not last.

To test what explaining you can use a ReentrantLock with fairness to true, meaning that the oldest waiting thread will be waked up first. Change RunCountAtomic as follows:

public class RunCountAtomic {
    private static final ReentrantLock lock = new ReentrantLock(true);
    public static AtomicInteger aValue = new AtomicInteger(0);

    public void runLoop() {
        for (int i = 0; i < 5; i++) {
            doSomeWork();
            aValue.decrementAndGet();
        }
        printlnValue(aValue.get());
    }

    private void printlnValue(int value) {
        lock.lock();
        try {
            System.out.println(" Expected value is - (5 x 10 thread = -50) , actual is: " + value);
        } finally {
            lock.unlock();
        }
    }

    public void doSomeWork() {
        try {
            Thread.sleep(100);
        } catch (InterruptedException e) {}
    }
}

Now you are guaranteed to have -50 printed last.

Upvotes: 1

Related Questions