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

Yes, you are correct. AtomicInteger would not grant any benefit if all access to the object is synchronized (only one thread, at most, would be accessing its contents at any given moment).

As others pointed out, the use of AtomicInteger is best when you need thread-safe access to that variable, and you perform simple updates on it.
In this case, you have two compound operations, transfer_funds and withdraw. The former has three accesses, and the latter has two.

You would like these operations to be atomic themselves, ie, they appear to others as if they occur instantaneously, they can't be broken down in smaller operations. To achieve this, synchronized is necessary.

---

On a final note, I'd like to leave a (possibly) useful suggestion. You should assign each account an unique identifier. Why, you may ask, to prevent deadlocks.

Assume we've got two threads, T1 and T2, and two accounts, a1 and a2.

T1:

a1.transfer_funds(a2, 42);

T2:

a2.transfer_funds(a1, 00101010);

You may experience the following interleaving:

T1 -> a1.enoughFund(42)
T1 -> a1.withdraw(42)
T2 -> a2.enoughFund(00101010)
T2 -> a2.withdraw(00101010)
T1 -> a2.deposit(42)    // blocks on a2's monitor, because T2 already has it
T2 -> a1.deposit(00101010)    // same as above

Both threads become indefinitely waiting for each other, because all your methods are synchronized.

The solution, when assigning each account an identifier, would be, for instance:

public class Account {
    private int balance;
    private final int id;

    /* Not synchronized */
    public boolean transferFunds(Account acc, int amount) {
        if (id < acc.getId()) {
            synchronized (this) {
                synchronized (acc) {
                    return transfer(acc, amount);
                }
            }
        }
        else if (id > acc.getId()) {
            synchronized (acc) {
                synchronized (this) {
                    return transfer(acc, amount);
                }
            }
        }
        return true; // same id, transfering to self has no effect.
    }

    private boolean transfer(Account acc, int amount) {
        if (balance >= amount) {
            balance -= amount;
            // This is not synchronized, you may make it private.
            acc.depositUnsynchronized(amount);
            return true;
        }
        return false;
    }
}

The above achieves lock acquisition in an orderly fashion, so, no matter the case, all threads will try to acquire the account with lowest id first. If a transfer is going on in that account, no other transfer will happen until the first ends.

Answer 3

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.

Answer 4

All that an atomic data type is promising you is to give a lock-free but thread-safe access to its value. So, one of the valid reasons you would use an AtomicInteger over synchronized is when you need to only protect your update operation like

synchronized (lockObj) {
    myInt++; // slower than AtomicInteger
}

In this case, AtomicInteger.incrementAndGet() would be faster. But, if your synchronization scope is bigger than that and the increment is just a part of it then using a synchronized block with a non-atomic integer (that's protected within that block) is recommended.

Answer 5

Yes for both, it is a good idea to make it synchronized, and Atomic is not needed.

If you rely simply on Atomic instead of synchronization, you can have issues such as in this method:

    if (enoughFund(amount)){
        withdraw(amount);
        acc.deposit(amount);
        return true;
    }

because Atomic only guarantees that your integer will be safe from simultaneous access, which means enoughFund(amount) will be guaranteed to provide a correct value for amount even if it is being written by some other thread. However, Atomic alone does not guarantee that the value obtained at this line would be the same as in the next line of code, because the other thread could execute another Atomic operation in between these two lines, resulting in withdraw(amount); being able to set your balance below zero.