not able to control atomic behavior of AtomicInteger while subtracting value

Question

This is the first time that I was using java.util.concurrent.atomic package for synchronization purpose in multithreading. I was trying to handle the classic example of AccountDanger class given in Kathy Sierra book for OCP through java.util.concurrent.atomic. But I was unable to protect the balance variable i.e AtomicInteger from being getting over withdrawn. Below is my code:

Account class

package p1;

import java.util.concurrent.atomic.AtomicInteger;

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

  public int getBalance() {
    return balance.get();
  }

  public void withdraw(int amount) {
    balance.addAndGet(-amount);
    System.out.println("~~~~~~ " + balance);
  }
}

AccountDanger class

package p1;

public class AccountDanger implements Runnable {
  private Account account = new Account();
  private int amt = 10;

  public void run() {

    for (int i = 0; i < 10; i++) {
      if (account.getBalance() >= amt) {
        System.out.println(Thread.currentThread().getName() + " is going to withdraw..");
        try {
          Thread.sleep(500);
        } catch (InterruptedException e) {
          // TODO Auto-generated catch block
          e.printStackTrace();
        }
        account.withdraw(amt);
      } else {
        System.out.println("not enough balance");
      }
      if (account.getBalance() < 0) {
        System.out.println("account is over withdrawn!!!");
      }
    }
  }

  public static void main(String[] args) throws InterruptedException {
    AccountDanger ad = new AccountDanger();
    Thread t1 = new Thread(ad, "Mark");
    Thread t2 = new Thread(ad, "Phew");
    t1.start();
    t2.start();
    t1.join();
    t2.join();
    System.out.println("final balance left is : " + ad.account.getBalance());
  }
}

I know, I am definitely wrong somewhere. can any one rectify my solution..

Answer 1

You're not using AtomicInteger (or, rather, Account) atomically.

if(account.getBalance()>=amt){
   // Other stuff
   account.withdraw(amt);

Other threads can change the balance inside that "other stuff".

In Account, you need a method like this, using AtomicInteger.compareAndSet:

boolean withdrawIfBalanceEquals(int balance, int amt) {
  return this.balance.compareAndSet(balance, balance - amt);
}

This only sets the AtomicInteger to balance-amt if its value is still equal to balance, and then returns true. If it has been changed since you read its value, compareAndSet will return false.

Then, in AccountDanger, you would use it like:

int balance = account.getBalance();
if (balance >= amt) {
  // Other stuff.
  account.withdrawIfBalanceEquals(balance, amt);
}

I leave it up to you to decide how best to handle the case of withdrawIfBalanceEquals returning false; one way is:

while (true) {
  int balance = account.getBalance();
  if (balance >= amt) {
    // Other stuff.
    if (account.withdrawIfBalanceEquals(balance, amt)) {
      // If true, we don't have to keep looping: we withdrew the
      // balance.
      break;
    }

    // If false, keep going: you'll read the balance again,
    // check if it's still greater than balance etc.

  } else {
    System.out.println("Not enough balance");
    break;
  }
}

Answer 2

The problem is that between the point in time where you check the balance (if(account.getBalance()>=amt) {) and the point in time where you actually withdraw (account.withdraw(amt);), the balance might have changed, e.g. lowered to an amount <= amt.

Let's look at a concrete scenario for clarity. If you have an account with a balance of 500 and you start two threads to withdraw 300 each, every transaction by itself is legal, since it does not overdraw, thus both checks of the if-statement will yield true. But the transactions do not know of each other. The first transaction lowers the balance by 300 to a value of 200. The second transaction then lowers the balance to -100.

To fix this problem, you need to introduce some synchronization. One possibility would be to check the balance before withdrawal:

public void withdraw (int amount) {
    if (balance.get() > amount) {
        balance.addAndGet(-amount);
        System.out.println("~~~~~~ "+balance);
    } else {
        throw new InsufficientBalanceException();
    }
}

As you can see, I introdcued an Exception to signal that the transaction could not be executed. You are free to introduce your own code for this case. This shortens the time between the check and the actual change, but does not fix the problem entirely. For this, you have to ensure that while one thread checks and possibly modifies the balance, no other thread can. One way to enforce this is the synchronized keyword:

public synchronized void withdraw(int amount) {
    if (balance.get() > amount) {
        balance.addAndGet(-amount);
        System.out.println("~~~~~~ "+balance);
    } else {
        throw new InsufficientBalanceException();
    }
}

Now, only one thread can call withdraw(...) on one instance at a time.

As @AndyTurner pointed out, if you already have a thread-safe data structure, like an AtomicInteger and have to synchronize around it, there might be something fishy. Each single operation on an AtomicInteger are atomic, but not multiple operations in sequence, which is why we had to introduce some synchronization. In this particular case, you can get rid of the AtomicInteger and replace it with a volatile int.