Synchronization while using AtomicInteger

Question

Let's assume that I want to implement a very simple Bank Account class , and we want to take care about concurrency and multi-threading issues,

Is it a good idea to make the following methods synchronized even though balance is AtomicInteger ?

On the other hand , if we have all the methods as synchronized , there would be no use of AtomicInteger anymore , right ?

import java.util.concurrent.atomic.AtomicInteger;


public class Account {
    AtomicInteger balance;
    public synchronized int checkBalance(){
        return this.balance.intValue();
    }
    public synchronized void deposit(int amount){
        balance.getAndAdd(amount);
    }
    public synchronized boolean enoughFund(int a){
        if (balance.intValue() >= a)
            return true;
        return false;
    }
    public synchronized boolean transfer_funds(Account acc, int amount){ // dest : acc 
        if (enoughFund(amount)){
            withdraw(amount);
            acc.deposit(amount);
            return true;
        }
        return false;
    }
    public synchronized boolean withdraw(int amount){
        if (checkBalance() < amount)
            return false;
        balance.getAndAdd(-1 * amount);
        return true;
    }
}

Answer 1

If you desperately wanted to use AtomicInteger, you could write:

public class Account {
    private final AtomicInteger balance = new AtomicInteger(0);

    public void deposit(int amount) {
        balance.getAndAdd(amount);
    }

    public boolean withdraw(int amount) {
        for (int i; i < SOME_NUMBER_OF_ATTEMPTS; ++i) {
            int currentBalance = balance.get();
            if (currentBalance < amount) return false;
            boolean updated = balance.compareAndSet(currentBalance, currentBalance - amount);
            if (updated) return true;
        }
    }

    public boolean transfer(int amount, Account recipient) {
        boolean withdrawn = withdraw(amount);
        if (withdrawn) recipient.deposit(amount);
        return withdrawn;
    }
}

That's safe, and it doesn't use locks. A thread making a transfer or a withdrawal isn't guaranteed to ever finish doing so, but hey.

The technique of looping around a compare-and-set is a standard one. It's how the locks used by synchronized are themselves implemented.

Answer 2

Having your amount declared as AtomicInteger does not prevent the thread from being preempted in the middle of method execution (if it's not synchronized). So for example if your method transfer_funds were not synchronized in any way, you could get unexpected results, even though your amount will be AtomicInteger

public /* synchronized */ boolean transfer_funds(Account acc, int amount){ // dest : acc 
        if (enoughFund(amount)){
            withdraw(amount);  // <- thread can be preempted in the middle of method execution
            acc.deposit(amount);
            return true;
        }
        return false;
    }

Such problems are called race conditions. One possible example is when two threads try to transfer funds from the same account. When one thread determines that there is enoughFund to perform credit transfer, that thread may be preempted and at the same time other thread can start transfering funds from this account. When the first thread starts processing again it does not double-check if there is enoughFunds to perform credit transfer (he already checked it, but his knowledge may be outdated), but it goes to the next line of execution. This way you may not get consistent results. The total amount that you had at the beginning on all accounts can be changed.

There is a very good explanation of this aspect in Cay Horstmann's Core Java book - here is chapter about synchronization available for free. It describes in detail almost exactly the same problem you are asking about.