Threaded bank transfer simulation and synchronization

I have following situation. I have a class for accounts, each account has a balance and money can be transfered to it.

public class Account {
    private int balance;

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

    public void transfer(int amount) {
        this.balance += amount;
    }

    @Override
    public String toString() {
        return "Account (balance: " + balance + ")";
    }
}

And I have a transfer manager. A transfer simply takes two accounts and a amount of money to be transfered. The transfer manager can issue transfers by adding it to the array list, kind of transfer queue. After all transfers are added to the queue the performTransfers method can be called which calles the performTransfer method on every transfer.

import java.util.ArrayList;

public class TransferManager {
    private ArrayList<Transfer> openTransfers;
    private int issuedTransfers;
    private int performedTransfers;

    public TransferManager() {
        openTransfers = new ArrayList<Transfer>();
        issuedTransfers = 0;
        performedTransfers = 0;
    }

    public void issueTransfer(Account from, Account to, int amount) {
        openTransfers.add(new Transfer(from, to, amount));
        issuedTransfers++;
    }

    public void performTransfers() {
        for(Transfer transaction : openTransfers) {
            transaction.performTransfer();
            performedTransfers++;
        }       
        openTransfers.clear();
    }

    @Override
    public String toString() {
        return "TransferManager (openTransfers: " + openTransfers.size() + "; issuedTransfers: " + issuedTransfers + "; performedTransfers: " + performedTransfers + ")";
    }

    private static class Transfer {
        private Account from, to;
        private int amount;

        public Transfer(Account from, Account to, int amount) {
            this.from = from;
            this.to = to;
            this.amount = amount;
        }

        public void performTransfer() {
            from.transfer(-amount);
            to.transfer(amount);
        }
    }
}

Now I add multithreading:

import java.util.Random;

public class BankingTest extends Thread {
    private Account[] accounts;
    private static Random random = new Random();

    public BankingTest(Account[] accounts) {
        this.accounts = accounts;
    }

    public void run() {
        final TransferManager manager = new TransferManager();

        //simulate some transfers
        for(int i = 0; i < accounts.len开发者_高级运维gth; i++) {
            final int index = i;
            Thread thread = new Thread() {
                public void run() {
                    try {
                        for(int j = 0; j < 10; j++) {
                            manager.issueTransfer(accounts[index], accounts[(index+1)%accounts.length], 100);
                            Thread.sleep(random.nextInt(10));
                        }
                    } catch (InterruptedException e) {                      
                        e.printStackTrace();
                    }
                }
            };
            thread.start();
        }

        //wait a bit
        try {
            Thread.sleep(60);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        manager.performTransfers();

        System.out.println(manager);
    }
}

BankingTest takes an array of say 10 blank accounts. For now it is not synchronized and I try to understand why I get those errors:

Exception in thread "Thread-2" java.lang.NullPointerException
at
gp2.ha5.exercise2.TransferManager.performTransfers(TransferManager.java:23)
at gp2.ha5.exercise2.BankingTest.run(BankingTest.java:41)

and

TransferManager (openTransfers: 0; issuedTransfers: 99; performedTransfers:
99)

Any idea why I get those errors and how can synchronization help here?

(you can zoom in to see the details ;))

TransferManager: http://pastebin.com/Je4ExhUz

BankTest: http://pastebin.com/cdpWhHPb

Start: http://pastebin.com/v7pwJ5T1

After I added the synchronized to issueTransfer and performTransfers method I keep getting errors:

Ok simple, all threads, try to execute this method :

public void issueTransfer(Account from, Account to, int amount) {
    openTransfers.add(new Transfer(from, to, amount));
    issuedTransfers++;
}

But there is no synchronization while adding to the ArrayList. You must understand that because list operation are not atomic and because several thread accessed it at the same time, almost anything can appen and that you list become corrupted.

Then when you try to read you list, you find Null elements inside it, even you didn't requested to insert one in the first place. This is an exemple of how accessing the same data without proper handling can corrupt your data.

EDIT :

So whenever you have shared state and you want to access it using several threads, you have to synchronize. This not only for the issueTransfer method.

Another problem is how you spawn threads. This is not related to your initial problem.

    //simulate some transfers
    for(int i = 0; i < accounts.length; i++) {
        final int index = i;
        Thread thread = new Thread() {
            public void run() {
                try {
                    for(int j = 0; j < 10; j++) {
                        manager.issueTransfer(accounts[index],

accounts[(index+1)%accounts.length], 100); Thread.sleep(random.nextInt(10)); } } catch (InterruptedException e) {

                    e.printStackTrace();
                }
            }
        };
        thread.start();

From the code here all threads access one global state : the index to use for accessing accounts in your array. But the main thread increment the with the for loop, while thread may execute at any time. There is a high risk that by the time the threads are started, index value has changed.

As you can see, concurrency always bite you when you don't take enough attention to it ;)

On top of what Nicolas said - it is not enough to synchronize issueTransfer, as main thread might already be in performTransfers and some threads can still be stuck in issueTransfer. This means your ArrayList is still accessed by multiple threads.

You can create a lock object and use

synchronized (lock) {
   // vulnerable code goes here
}

to protect those code snippets that can potentially be touched by multiple threads. Alternatively you can make relevant methods (issueTransfer and performTransfers) synchronized.