Bank Simulator with Synchronization in Java

Question

I have a method transfer() which withdrawals money from one account and deposits it into another. There are 10 accounts each running with their own thread. I have another method test() which sums up the total in each account to make sure that the bank has not lost or gained money. In order to have an accurate total, I created a boolean flag to indicate if testing is in progress. If it is, I need to somehow suspend the transfers until the test is finished. I've tried to implement this using a synchronized block to tell the threads to wait on a condition and release once the condition is no longer true. For some reason I'm having difficulty. My transfer method looks like this:

public class Bank {

    public static final int NTEST = 10;
    private Account[] accounts;
    private long ntransacts = 0;
    private int initialBalance;
    private int numAccounts;
    private boolean open;
    private int transactsInProgress;
    private boolean testing=false;

    public Bank(int numAccounts, int initialBalance) {
        open = true;
        this.initialBalance = initialBalance;
        this.numAccounts = numAccounts;
        accounts = new Account[numAccounts];
        for (int i = 0; i < accounts.length; i++) {
            accounts[i] = new Account(this, i, initialBalance);
        }
        ntransacts = 0;
        transactsInProgress = 0;
    }
    public synchronized void incrementTransacts(){
        transactsInProgress++;
    }
    public synchronized void decrementTransacts(){
        transactsInProgress--;
    }

    public void transfer(int from, int to, int amount) throws InterruptedException {

    accounts[from].waitForAvailableFunds(amount);
    synchronized(this){
        while(testing){
            System.out.println("Cannot transfer while testing...");
            this.wait();
        }
    }
        if (!open) return;
        if (accounts[from].withdraw(amount)) {
            incrementTransacts(); //synchronzied method increments transactsInProgress
            accounts[to].deposit(amount);
            decrementTransacts(); //synchronized method
        }
        if (shouldTest()) test();

    synchronized(this){
        this.notifyAll();
    }    
    }

    public synchronized void test() throws InterruptedException {
        int sum = 0;

        testing=true;
        while(transactsInProgress!=0){
                System.out.println("Cannot test while transactions are in progres... \nWaiting...");
            wait();
        }

        for (int i = 0; i < accounts.length; i++) {
            System.out.printf("%s %s%n", 
                    Thread.currentThread().toString(),accounts[i].toString());
            sum += accounts[i].getBalance();
        }
        System.out.println(Thread.currentThread().toString() + 
                " Sum: " + sum);
        if (sum != numAccounts * initialBalance) {
            System.out.println(Thread.currentThread().toString() + 
                    " Money was gained or lost");
            System.exit(1);
        } else {
            System.out.println(Thread.currentThread().toString() + 
                    " The bank is in balance");
        }
        testing=false;
        notifyAll();
    }
       public int size() {
        return accounts.length;
    }

    public synchronized boolean isOpen() {return open;}

    public void closeBank() {
        synchronized (this) {
            open = false;
        }
        for (Account account : accounts) {
            synchronized(account) {
                account.notifyAll();
            }
        }
    }

    public synchronized boolean shouldTest() {
        return ++ntransacts % NTEST == 0;
    }
}

It's been a while since I've coded in Java and I'm new to threads and concurrency so I'm not sure exactly where I'm going wrong. When I run the program, the bank sum is incorrect. Theres 10,000 in each account so the sum each time should be 100,000. Any ideas here?

EDIT: The thread class and Main:

class TransferThread extends Thread {

    public TransferThread(Bank b, int from, int max) {
        bank = b;
        fromAccount = from;
        maxAmount = max;
    }

    @Override
    public void run() {
        for (int i = 0; i < 10000; i++) {
            int toAccount = (int) (bank.size() * Math.random());
            int amount = (int) (maxAmount * Math.random());
            bank.transfer(fromAccount, toAccount, amount);
        }
        bank.closeBank();
    }
    private Bank bank;
    private int fromAccount;
    private int maxAmount;
}

Main:

public static void main(String[] args) throws InterruptedException {
    Bank b = new Bank(NACCOUNTS, INITIAL_BALANCE);
    Thread[] threads = new Thread[NACCOUNTS];
    // Start a thread for each account
    for (int i = 0; i < NACCOUNTS; i++) {
        threads[i] = new TransferThread(b, i, INITIAL_BALANCE);
        threads[i].start();
    }
    // Wait for all threads to finish
    for (int i = 0; i < NACCOUNTS; i++) {
        try {
            threads[i].join();
        } catch (InterruptedException ex) {
            // Ignore this
        }
    }
    b.test();
}

Answer 1

I don't know your exact issue, but there are a few concerning things in your code:

1. Your transfer() method has two different synchronized blocks, but appears to perform operations which should be protected between them.

2. Don't trust primitive boolean variables for synchronization. When you're working with multiple threads you should use AtomicBoolean.

Update now that I understand the problem a little better:

The issue here is that you're using trying to use synchronized in a manner which wasn't intended by its designers. If you're going to synchronize, you pick an object and say "only one thread can manipulate this thing at a time". Using synchronized(this) or declaring methods as synchronized in your Bank class says "only one thread can manipulate the state of the bank at once".

From your comment below, I understand that's not the case. If multiple threads can update accounts at once, the bank isn't the resource you want to synchronize on.

You should either protect at a more granular level (e.g., locking each account individually), or use a different lock construct such as ReadWriteLock which allows multiple threads to say share a lower level of access or a single thread to gain exclusive access.

Answer 2

The bug is not immediately apparent however this looks wrong:

    if (accounts[from].withdraw(amount)) {
        incrementTransacts(); //synchronzied method increments transactsInProgress
        accounts[to].deposit(amount);
        decrementTransacts(); //synchronized method
    }

This section of code is not synchronized. If 2 threads are trying to deposit to the same account, they might overwrite each other. Or if there aren't any memory barriers, the correct balance may not be published correctly. You aren't showing the Account object but if it is not thread-safe, that's probably the source of the issue.

Each account could have an AtomicInteger balance that would then be updated atomically and in a thread-safe manner.

class Account {
   private final AtomicInteger balance = new AtomicInteger(0);
   ...
   public int withdraw(int amount) {
        // loop to get the money in a thread-safe manner
        while (true) {
           // get current balance
           int current = balance.get();
           if (current < amount) {
               // not enough funds
               return 0;
           }
           // update the balance atomically if it is still current
           if (balance.compareAndSet(current, current - amount)) {
               return amount;
           }
           // someone else beat me to it so loop and get new balance
       }
   }

   public void deposit(int amount) {
       // similar but without the not-enough-funds check and with a +
   }

Here are some other issues in your code that might help.

1. accounts[from].waitForAvailableFunds(amount);

   You are waiting for funds and then withdrawing them. There is a race condition there that might cause another 2 threads to both return true but only one thread would actually get the funds. We also can't see the Account class to make sure that it is thread-safe.

   I would have some sort of accounts[from].waitAndWithdraw(...) method which would wait until the account has funds and then withdraw it.

2. It seems to me that you could deadlock your system pretty easily if you get the right combination of withdraw and deposit commands. For example, if 10 withdraws were happening, none of them might have the funds and your system would just stop. You could do a wait(...) with a timeout and return an error code (or throw) if the timeout is reached.

   This won't happen from your main(...) as written but other users could do it.

3. public synchronized void decrementTransacts(){ transactsInProgress--; }

   This sort of code is begging for use of AtomicInteger. Then you can do transactsInProgress.incrementAndGet() without the sync lock.

4. As @Tyler mentions, you might consider synchronizing instead on the Account objects instead of the Bank. So the Bank would just be the class that issues the transfer but the locks would be localized on the Account objects to make sure they are thread-safe. Or with the above code, many of the sync locks may not be necessary.

5. It is pretty obvious that there are times that the test() is going to fail since there are times when you have withdrawn an amount from one account but may not have deposited the same amount in the other account.