Adding money from one account to another using multithreading

I have 2 accounts and 2 threads. 1 thread transfers money from 1 account to 2 account, 2 thread transfers money from 2 account to 1 account, if of course there is enough money. I need to realize the deadlock situation and solve the deadlock situation confirming safe transfer. Here is what I have for now:

Account.java

public class Account {

    private /*volatile*/ long balance;

    public Account() {
        this(0L);
    }

    public Account(long balance) {
        this.balance = balance;
    }

    public long getBalance() {
        return balance;
    }

    public synchronized void deposit(long amount) {
        checkAmountNonNegative(amount);
        balance += amount;
    }

    public synchronized void withdraw(long amount) {
        checkAmountNonNegative(amount);
        if (balance < amount) {
            throw new IllegalArgumentException("not enough money");
        }
        balance -= amount;
    }

    private static void checkAmountNonNegative(long amount) {
        if (amount < 0) {
            throw new IllegalArgumentException("negative amount");
        }
    }
}

Main.java

public class Main {

    public static void main(String[] args) {
        Account first = new Account(1_000_000);
        Account second = new Account(1_000_000);

        TransferThread thread1 = new TransferThread(first, second, 2000);
        TransferThread thread2 = new TransferThread(second, first, 2000);

        CompletableFuture.allOf(
                CompletableFuture.runAsync(thread1),
                CompletableFuture.runAsync(thread2)
        ).join();

        System.out.println(first.getBalance());
        System.out.println(second.getBalance());
    }
}

TransferThread.java

public class AccountThread implements Runnable {

    private final Account from;
    private final Account to;
    private final long amount;

    public AccountThread(Account from, Account to, long amount) {
        this.from = from;
        this.to = to;
        this.amount = amount;
    }

    @Override
    public void run() {
        for (int i = 0; i < 2000; i++) {
            // my realization
            try {
                if (from.getBalance() < 0) {
                    throw new InsufficientFundsException();
                } else {
                    from.deposit(amount);
                    to.withdraw(amount);
                }
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }

I decided to put synchronized to both methods deposit and withdraw for safe transfering. But doubt with the method run realization. Do I have a rigth implementation? If not, explanation and correction will be appreciated.

Answer

Your solution is not vulnerable to deadlocks because the TransferThread instances are never going to hold more than one lock at a time.

However, I don’t think it is a correct solution. The problems are here:

if (from.getBalance() < 0) {
    throw new InsufficientFundsException();
} else {
    from.deposit(amount);
    to.withdraw(amount);
}

The first problem is you are transferring money in the wrong direction! Money should go from the from account to the to account. But you are depositing money into the from account and withdrawing money from the to account.

Customers won’t be happy about that.

Lets fix that:

if (from.getBalance() < 0) {
    throw new InsufficientFundsException();
} else {
    to.deposit(amount);
    from.withdraw(amount);
}

The problem now is that we are depositing before we withdraw. Why is that a problem? Because between the from.getBalance() and from.withdraw(...) calls, another thread could make a withdrawl from the from account. That could mean that our from.withdraw(amount) call could fail. But we’ve already deposited the money into the to account. Ooops!

Lets fix that:

if (from.getBalance() < 0) {
    throw new InsufficientFundsException();
} else {
    from.withdraw(amount);
    // HERE
    to.deposit(amount);
}

Close …

What happens if we get a power dip at the point marked HERE? Well, if we dealing with real bank accounts, then we would actually be storing the information in a database. So the values currently in the accounts would be preserved. But at the point labelled HERE we would have withdrawn money from one account and not deposited it in the other one. What happens to that money? Poof! Gone!

Does that matter? Well it depends on how we frame the requirements. Assuming that it is OK to represent the bank accounts as (only) in memory objects, I’d say that we can ignore that nicety of surviving power failure during a transfer. The power failure would blow away the accounts as well.

Close enough is good enough, here. But we can do slightly better. As we noted, the value in the from account can change between the getBalance() and withdraw() calls, so that the withdraw() could fail. But when you think about it from.withdraw is just testing from.getBalance() < 0 anyway. So, we can just get rid of the test:

    from.withdraw(amount);
    to.deposit(amount);

If the from.withdraw(amount) is going to overdraw the account, it will fail with an exception. And we won’t then do the to.deposit(amount) call.

---

Now we could try to implement a transfer method that takes two accounts as arguments, and transfers money from one to the other as an atomic operation. You could conceivably do this by acquiring locks on both accounts before doing the transfer; e.g. like this:

  public static void transfer(Account from, Account to, long amount {
      synchronized (from) {
          synchronized (to) {
              from.withdraw(amount);
              to.deposit(amount);
          }
      }
  }

^{(I’m deliberately ignoring the exceptions and exception handling.)}

But now we have to worry about deadlock. For example, if one thread tries to transfer money from A to B, and another simultaneously transfer money from B to A.

There are ways to deal with this:

• One way is to the Lock API and acquire the locks with a timeout to detect deadlock.

• Another way is write the transfer method to acquire the account locks in the same order when doing transfer(A,B) and transfer(B,A). For example, assuming that Account objects have unique account numbers, then lock the Account with the lower account number first.

    public static void transfer(Account from, Account to, long amount {
        if (from.getAccountNo() < to.getAccountNo()) {
            synchronized (from) {
                synchronized (to) {
                    from.withdraw(amount);
                    to.deposit(amount);
                }
            }
        } else {
            synchronized (to) {
                synchronized (from) {
                    from.withdraw(amount);
                    to.deposit(amount);
                }
            }
        }
    }