Better solution instead of nested synchronized blocks in Java?

Question

I have a Bank class with a list of Account. The bank has a transfer() method to transfer a value from one account to another. The idea is to lock both the from and to accounts within a transfer.

To solve this issue I have the following code (please bear in mind that this is a very trivial example because it's just that, an example):

public class Account {
    private int mBalance;

    public Account() {
        mBalance = 0;
    }

    public void withdraw(int value) {
        mBalance -= value;
    }

    public void deposit(int value) {
        mBalance += value;
    }
}

public class Bank {
    private List<Account> mAccounts;
    private int mSlots;

    public Bank(int slots) {
        mAccounts = new ArrayList<Account>(Collections.nCopies(slots, new Account()));
        mSlots = slots;
    }

    public void transfer(int fromId, int toId, int value) {
        synchronized(mAccounts.get(fromId, toId)) {
            synchronized(mAccounts.get(toId)) {
                mAccounts.get(fromId).withdraw(value);
                mAccounts.get(toId).deposit(value);
            }
        }
    }
}

This works, but does not prevent deadlocks. To fix that, we need to change the synchronization to the following:

synchronized(mAccounts.get(Math.min(fromId, toId))) {
    synchronized(mAccounts.get(Math.max(fromId, toId))) {
        mAccounts.get(fromId).withdraw(value);
        mAccounts.get(toId).deposit(value);
    }
}

But the compiler warns me about nested synchronization blocks and I trust that that is a bad thing to do? Also, I'm not very fond of the max/min solution (I was not the one who came up with that idea) and I would like to avoid that if possible.

How would one fix those 2 problems above? If we could lock on more than one object, we would lock both the from and to account, but we can't do that (as far as I know). What's the solution then?

Answer 1

I personally prefer to avoid any but the most trivial synchronization scenario. In a case like yours I would probably use a synchronized queue collection to funnel deposits and withdraws into a single-threaded process that manipulates your unprotected variable. The "Fun" thing about these queues is when you put all the code into the object that you drop into the queue so the code pulling the object from the queue is absolutely trivial and generic (commandQueue.getNext().execute();)--yet the code being executed can be arbitrarily flexible or complex because it has an entire "Command" object for it's implementation--this is the kind of pattern that OO-style programming excels at.

This is a great general-purpose solution and can solve quite a few threading problems without explicit synchronization (synchronization still exists inside your queue but is usually minimal and deadlock-free, often only the "put" method needs to be synchronized at all, and that's internal).

Another solution to some threading problems is to ensure that every shared variable you might possibly write to can only be "Written" to by a single process, then you can generally leave off synchronization altogether (although you may need to scatter a few transients around)

Answer 2

Lock ordering is indeed the solution, so you're right. The compiler warns you because it cannot make sure all your locking is ordered—it's not smart enough to check your code, and smart enough to know there may be more.

An alternative solution could be locking on an enclosing object, e.g. for transfers within one user's account you could lock on user. Not so with transfers between users.

Having said that, you are not probably going to rely on Java locking in order to make a transfer: you need some data storage, usually a database. In case of using a database, the locking moves to the storage. Still, the same principles apply: you order locks to avoid deadlocks; you escalate locks to make locking simpler.