AtomicInteger not reading value from main memory for non-volatile mutable refrence

Question

Below is a non-thread-safe implementation of popular Husband Wife bank account problem.

(One thread first checks the account, and before the same thread performs withdraw, another thread performs withdraw operation, thus breaking the code).

If we look at the logs of the program after execution of Demo.java file. It is clear that, "Wife Thread" is not reading value of AtomicInteger amount from main memory.

Also, I tried the same example with plain "volatile int". But again, I am facing the same problem :- "Wife-Thread is not reading value of amount integer from main memory."

Kindly explain this behaviour to help me understand the concept. Please find the code below :-

AtomicBankAccount.java

package pack;

import java.util.concurrent.atomic.AtomicInteger;

public class AtomicBankAccount {

    private AtomicInteger amount ;

    public AtomicBankAccount(int amt) {
        this.amount = new AtomicInteger(amt) ;
    }

    // returns
    // -1 for insufficient funds
    // remaining balance without subtracting from actual amount for sufficient funds
    public int check(int amtToWithdraw){

        if(amtToWithdraw <= amount.get()){
            System.out.println(Thread.currentThread().getName() + " checks amount : " + amount.get() + ". Remaining ammount after withdrawl should be : " + (amount.get() - amtToWithdraw));
            return (amount.get() - amtToWithdraw) ;
        }else{
            return -1 ;
        }
    }

    // returns
    // remaining balance after subtracting from actual amount
    public int withdraw(int amtToWithdraw){
        amount.getAndAdd(-amtToWithdraw) ;
        System.out.println(Thread.currentThread().getName() + " withdraws " + amtToWithdraw + ". Remaining : " + amount.get() + " [latest updated value of account in main memory]");
        return amount.get() ;
    }

    public int getAmount(){
        return amount.get() ;
    }
}

AtomicWithdrawThread.java

package pack;

public class AtomicWithdrawThread extends Thread{ 

    private AtomicBankAccount account ;

    public AtomicWithdrawThread(AtomicBankAccount acnt, String name) {
        super(name) ;
        this.account = acnt ;
    }

    @Override
    public void run() {
        int withDrawAmt = 2 ;
        int remaining = 0 ;
        while(true){

            if( (remaining = account.check(withDrawAmt)) != -1 ){
                int temp = account.withdraw(withDrawAmt) ;
                if(temp != remaining){
                    System.out.println("[Race condition] " + Thread.currentThread().getName());
                    System.exit(1) ;
                }
            }else{
                System.out.println("Empty Account....");
                System.exit(1) ;
            }
        }
    }
}

Demo.java

package pack;

public class Demo {

    public static void main(String[] args) {
        AtomicBankAccount bankAccount = new AtomicBankAccount(1000) ;

        AtomicWithdrawThread husbandThread = new AtomicWithdrawThread(bankAccount, "husband") ;
        AtomicWithdrawThread wifeThread = new AtomicWithdrawThread(bankAccount, "wife") ;

        husbandThread.start() ;
        wifeThread.start() ;
    }
}

Best Regards,

rits

Answer 1

That code looks fishy:

amount.getAndAdd(-amtToWithdraw) ;
return amount.get() ;

If the other thread creeps between that... funny things can happen. Use and test that code instead (also in the System.out please):

int amt = amount.getAndAdd(.amtToWithdraw);
return amt - amtToWithdraw;

And here also:

   if(amtToWithdraw <= amount.get()){
       return (amount.get() - amtToWithdraw) ;

use again the pattern

    int amt = amount.get();
    if(amtToWithdraw <= amt){
        return (amt - amtToWithdraw) ;

But that code is NOT fixable:

        if( (remaining = account.check(withDrawAmt)) != -1 ){
            int temp = account.withdraw(withDrawAmt) ;

Between those accesses to the AtomicInteger the other thread can creep in and wreck havok. You must adapt the code to be thread safe.

A usual pattern/idiom is like this:

    // In AtomicBankAccount
    public int withdraw(int amtToWithdraw){
        for(;;){
            int oldAmt = amount.get();
            int newAmt = oldAmt - amtToWithdraw;
            if( newAmt < 0 )
                return -1;
            if( amount.compareAndSet(oldAmt, newAmt) ){
                System.out.println(Thread.currentThread().getName() + " withdraws " + amtToWithdraw + ". Remaining : " + newAmt + " [latest updated value of account in main memory]");      
                return newAmt;
            }
        }
    }

    // in AtomicWithdrawThread:
    public void run() {
        int withDrawAmt = 2 ;
        while(true){
            if( account.withdraw(withDrawAmt) >= 0 ){
                // OK
            }
            else{
                System.out.println("Empty Account....");
                System.exit(1) ;
            }
        }
    }

Note that checkWithdraw is no more. That`s good because that way no one else can get get between the check and the actual withdrawal.

Answer 2

Note: These first few paragraphs describe the deliberate lack of thread safety in the question, and don't actually answer the point the questioner was asking about..

The check method and the withdraw method, while individually are atomic, don't combine into a single atomic operation.

Say Husband checks the accounts, finds there is enough remaining, and then gets suspended.

Wife checks the accounts, then withdraws the remaining money.

Husband is then allowed to continue, and tries to withdraw the money, but finds wife has already gone off with it all.

Edit: Describes the reason for the questioner's issue

You aren't calling System.out in a thread-safe way. There is a race condition between calculating the message that you are going to display and actually getting it to appear on the console - so the wife's message is probably calculated before the husband's withdrawal, but displayed after it.

You need to add synchronized keywords around those System.out lines (or something equivalent) if you want to eliminate this effect.

Just imagine your code actually looks like this:

String message = Thread.currentThread().getName() + " checks amount : " + amount.get() + ". Remaining ammount after withdrawl should be : " + (amount.get() - amtToWithdraw);
System.out.println(message);

Does this help show where the race condition is?