synchronize concept in java doesn't work?

Question

We have one hundred accounts in the bank and two clerks, implemented as threads, who transfer each 1000 times money from account with number accountNumberFrom to account accountNumberTo, using the synchronized method transferMoney. Since all accounts start with balance 0 and the money retrieved from one account is transferred to another, the balance should be zero after all the transactions. This is true most of the time, but not always. Although it occurs seldom, but sometimes the balance after the transactions is not equal to 0. What is wrong?

public class Clerk extends Thread {
    private Bank bank;

    public Clerk(String name, Bank bank) {
        super(name);
        this.bank=bank;
        start();
    }

    public void run() {
        for (long i=0; i<1000; i++) {
            int accountNumberFrom = (int) (Math.random()*100);
            int accountNumberTo = (int) (Math.random()*100);
            float amount = (int) (Math.random()*1000) - 500;
            bank.transferMoney(accountNumberFrom, amount);
            bank.transferMoney(accountNumberTo, -amount);
        }
    }
}

and a class Bank

public class Bank {
    Account[] account;

    public Bank() {
        account = new Account[100];
        for (int i=0; i < account.length; i++)
            account[i] = new Account();
    }

    public synchronized void transferMoney(int accountNumber, float amount) {
        float oldBalance = account[accountNumber].getBalance();
        float newBalance = oldBalance + amount;
        account[accountNumber].setBalance(newBalance);
    }
}

public class Banking {
    public static void main (String[] args) {
        Bank myBank = new Bank();
        /**
         * balance before transactions
         */
        float sum=0;
        for (int i=0; i<100; i++)
            sum+=myBank.account[i].getBalance();
        System.out.println("before: " + sum);

        new Clerk ("Tom", myBank);
        new Clerk ("Dick", myBank);        

        /**
         * balance after transactions
         */
        for (int i=0; i<100; i++)
            sum+=myBank.account[i].getBalance();

        System.out.println("after: " + sum);
    }
}

Answer 1

One issue is that the synchronized transferMoney method only takes one account and so it's possible another thread could access the account balances after the transfer amount has been added to the "to" account but before it has been deducted from the "from" account. If all accounts start at zero, we could have this sequence of events:

1. Clerk Tom adds $100 to account 1.
2. Main thread totals account balances.
3. Clerk Tom deducts $100 from account 2.

At step 2 we see that the total of all accounts is $100 instead of zero.

So it's important that the transferMoney method update both accounts while holding the synchronized lock.

Another issue is that while the transferMoney is synchronized, the code that totals the account balances (step 2 above) is not. So even if you update both accounts in the transferMoney method, the sequence of events above can still happen because the main thread doesn't synchronize before executing step 2.

I would move the code that totals up the accounts to Bank and make it synchronized too. That will make both methods synchronized on the Bank instance and prevent the sequence of events that gives the wrong error.

A secondary issue is that in the main thread, you don't wait for the clerks to finish their transfers. Your code is performing all 1,000 transfers, but you just check the balances right after you start the clerk threads, so you may be looking at the balances after 0 transfers, or after all 1,000, or after 639 transfers, who knows. Doing the synchronization correctly will prevent you from seeing a non-zero total balance, but you should still wait for the clerks to finish. (Give it a try and if you can't figure it out, post a new question.)

Answer 2

In your example synchronized only block all thread call to myBank.transferMoney, but it doesn't ensure every threads done on main thread, you can update source code like this:

class Clerk extends Thread {
    private Bank bank;
    private volatile boolean done;

    public Clerk(String name, Bank bank) {
        super(name);
        this.done = false;
        this.bank=bank;
        start();
    }

    public void run() {
        for (long i=0; i<1000; i++) {
            int accountNumberFrom = (int) (Math.random()*100);
            int accountNumberTo = (int) (Math.random()*100);
            float amount = (int) (Math.random()*1000) - 500;
            bank.transferMoney(accountNumberFrom, amount);
            bank.transferMoney(accountNumberTo, -amount);
        }
        this.done = true;
    }

    public boolean isDone() {
        return done;
    }
}

class Account {

    protected float balance;

    public float getBalance() {
        return balance;
    }

    public void setBalance(float newBalance) {
        this.balance = newBalance;
    }

}

class Bank {
    Account[] account;

    public Bank() {
        account = new Account[100];
        for (int i=0; i < account.length; i++)
            account[i] = new Account();
    }

    public synchronized void transferMoney(int accountNumber, float amount) {
        float oldBalance = account[accountNumber].getBalance();
        float newBalance = oldBalance + amount;
        account[accountNumber].setBalance(newBalance);
    }
}

public class Banking {
    public static void main (String[] args) throws Exception {
        for(int j = 0 ; j < 1000 ; ++j) {
            Bank myBank = new Bank();
            /**
             * balance before transactions
             */
            float sum=0;
            for (int i=0; i<100; i++)
                sum+=myBank.account[i].getBalance();
            System.out.println("before: " + sum);

            Clerk a = new Clerk ("Tom", myBank);
            Clerk b = new Clerk ("Dick", myBank);

            while(!a.isDone() || !b.isDone()) // wait util all thread done
                Thread.sleep(1);

            /**
             * balance after transactions
             */
            for (int i=0; i<100; i++)
                sum+=myBank.account[i].getBalance();

            System.out.println("after: " + sum);
        }
    }
}

Answer 3

Thanks a lot for the helpful answers. I modified my code and now it is working as it should:

public class Bank
{
    Account[] account;

    public Bank() {
        account = new Account[100];
        for (int i=0; i < account.length; i++)
            account[i] = new Account();
    }

    public void transferMoney(int accountNumber, float amount) {
        synchronized (account[accountNumber]) {
            float oldBalance = account[accountNumber].getBalance();
            float newBalance = oldBalance - amount;
            account[accountNumber].setBalance(newBalance);
        }
    }
}

public class Account {
    private float balance;

    public void setBalance(float balance) {
        this.balance=balance;
    }

    public float getBalance() {
        return this.balance;
    }
}

public class Clerk extends Thread {
    private Bank bank;

    public Clerk(String name, Bank bank) {
        super(name);
        this.bank=bank;
    }

    public void run() {
        for (long i=0; i<100; i++) {
            int accountNumberFrom = (int) (Math.random()*100);
            int accountNumberTo = (int) (Math.random()*100);
            float amount = (int) (Math.random()*1000);
            bank.transferMoney(accountNumberFrom, -amount);
            bank.transferMoney(accountNumberTo, amount);
        }
    }
}

public class Accountant extends Thread
{
    Bank bank;

    public Accountant(String name, Bank bank)
    {
        super(name);
        this.bank=bank;
    }

    @Override public void run() {
        getBalance();
    }

    public synchronized void getBalance() {
        float sum=0;

        System.out.println(Thread.currentThread().getName());
        for (int i=0; i<100; i++)
            sum+=bank.account[i].getBalance();

        System.out.println("Bilanz: " + sum);
    }
}

public class Banking {

    public Banking() {
    }

    public static void main(String[] args) {
        Bank myBank = new Bank();
        Clerk tom = new Clerk ("Tom", myBank);
        Clerk dick = new Clerk ("Dick", myBank);        
        Accountant harry = new Accountant("Harry", myBank);

        tom.start();
        dick.start();

        try { 
            System.out.println("Current Thread: " + Thread.currentThread().getName()); 
            tom.join(); 
            dick.join(); 
        } 
        catch(Exception x) { 
            System.out.println("Exception has " + "been caught" + x); 
        }

        harry.start();
    }
}