1
\$\begingroup\$

I created a simple Bank application to make multiple transactions between multiple accounts. It is working as expected. But I want to know can I make the code more optimized.

public class BankingApplication {
    public static void main(String[] args) throws  Exception{
        ExecutorService service = Executors.newFixedThreadPool(100);
        Bank bankObj = new Bank();
        for(int i =0; i <= 10000; i++){
            service.submit(bankObj);
        }
        service.shutdown();
        service.awaitTermination(1, TimeUnit.DAYS);
        bankObj.getTotalBalanceWithStats();
    }
}
class Account{
    String accountName;
    int balance;
    Object lock1 = new Object();
    Object lock2 = new Object();
    Object lock3 = new Object();
    Account(String name, int balance){
        this.accountName = name;
        this.balance = balance;
    }
    void deposit(int amount){
        synchronized (lock1) {
            this.balance = getBalance() + amount;
        }
    }
    void withdraw(int amount) throws Exception {
        synchronized (lock2) {
            int balance = getBalance() - amount;
            if(balance <= 0){
                throw new Exception("Invalid Transaction");
            }
            this.balance = getBalance() - amount;
        }
    }
    int getBalance(){
        synchronized (lock3){
            return this.balance;
        }
    }
}
class Bank implements Runnable{

    List<Account> accountList = new ArrayList<>();
    Bank(){
        for(int i=0; i <= 30; i++){
            accountList.add(new Account("A"+i, 1000));
        }
        getTotalBalanceWithStats();
    }

    public void run() {
        Random random = new Random();
        Account from = null;
        Account to = null;
        while(true) {
            int a1 = random.nextInt(30);
            int a2 = random.nextInt(30);
            if(a1 == a2) continue;
            from = accountList.get(a1);
            to = accountList.get(a2);
            break;
        }
        doTransaction(from, to, random.nextInt(400));
    }

    private void doTransaction(Account from, Account to, int nextInt) {
        System.out.println("Transaction from " + from.accountName + " to " + to.accountName);
        try {
            from.withdraw(nextInt);
            to.deposit(nextInt);
        } catch (Exception e){
            System.out.println(e.getMessage());
        }
    }

    void getTotalBalanceWithStats(){
        int total = 0;
        for(Account acc : accountList){
            System.out.println("Account " + acc.accountName + " balance " + acc.getBalance());
            total += acc.getBalance();
        }
        System.out.println("Total Balance " + total);
    }
}

As per my knowledge I feel deposit(), withdrawal() and getBalanace() API are independent of each other so creating different mutex for each process.

Some scenarios which I am trying to test

What I am trying to achieve is when Account1 is trying to send money to Account2 at the same time Account3 can also send money to Account2 or vice versa.

When Account1 is sending money to Account2 at same time Account2 can also send money to Account1.

When Account1 is sending money to Account2 and at same time Account3 is sending money to Account1. withdraw() method of Account1 should not block deposit().

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview@SE. \$\endgroup\$ – greybeard Mar 11 at 6:01
  • \$\begingroup\$ (Out of curiosity: what have been the getTotalBalanceWithStats() outputs in your test runs?) \$\endgroup\$ – greybeard Mar 11 at 6:02
  • \$\begingroup\$ @greybeard I am getting the proper output everytime. \$\endgroup\$ – Harsh Kanakhara Mar 11 at 8:41
  • \$\begingroup\$ Banking is one application where accuracy and auditability are way more important than speed. \$\endgroup\$ – Gilbert Le Blanc Mar 30 at 2:51
5
\$\begingroup\$

This code is broken.

Imagine an Account with $100, two threads, one depositing $1, the other withdrawing $50.

Thread one acquired lock1, and begins executing:

this.balance = getBalance() + amount;

and reads $100 ...

Thread two acquired lock2, reads the balance (still $100), subtracts $50, stores the new balance ($50) and releases lock2.

... (back to thread one) ... adds $1, for a total of $101, which it stores in balance.

Free money! The withdrawal of $50 was forgotten.

You must lock all read-modify-write operations of an object's member with the same lock.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I got your point. But what should be the ideal way in which different threads acquire lock on different processes to make transaction faster. If Thread1 is sending money from A1 to A2 at same time A3 can send money to A1. I am unable to get irrespective of having different lock why I am getting correct output everytime. \$\endgroup\$ – Harsh Kanakhara Mar 11 at 8:47
  • \$\begingroup\$ For reason creating different mutex is when Thread1 is depositing amount for Account1 at same time Thread2 can withdraw amount if not less than 0. I am here handling the failover scenario. But at same time no two thread can deposit amount for Account1. I hope you got my point. \$\endgroup\$ – Harsh Kanakhara Mar 11 at 8:52
  • 1
    \$\begingroup\$ It is almost impossible to create unit tests for concurrency issues. You would have to be able to insert a delay between calling getBalance() and assigning the result of the addition/subtraction to balance. Your unit tests returning correct results are meaningless here. With a debugger you probably would be able to do it by manually stepping through statements in each thread. \$\endgroup\$ – TorbenPutkonen Mar 11 at 11:31
  • 1
    \$\begingroup\$ And I don't think you are getting the point, because the point is that you can not perform a withdrawal and a deposit to the same Account in parallel. It is not the place where you can optimize for performance. \$\endgroup\$ – TorbenPutkonen Mar 11 at 11:33
  • 1
    \$\begingroup\$ "You must lock all read-modify-write operations of an object's member with the same lock." As in, remove lock2 and lock3, changing usages to lock1. Or, maintain 2 members: balance and credits, with 1 lock for credits, and one for balance. Deposits lock & add to credits. Withdraws lock & subtract from balance, but if balance is about to go negative, then also lock credits, transfer credits into balance, zeroing credits, & unlock credits. Then attempt to subtract from balance again. Finally unlock balance. I doubt that gains you anything, however. \$\endgroup\$ – AJNeufeld Mar 11 at 13:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.