CODEWITHSUNDEEP
javaconcurrency
user384842
user384842

Reputation: 1975

Is this code correctly using ReentrantReadWriteLock?

I'm familiar with concurrency examples that use synchronized when transferring money between one account and another, in which the locking of the two accounts is ordered on the account number, for example, so that deadlocks cannot occur.

I wanted to explore using ReentrantReadWriteLock, because it seemed to me that this would allow clients of an Account object to do concurrent reads, provided no client was updating the object.

I've written the code and tested it, and it seems to work, but it looks a bit ugly, for example, it looks a bit strange for the Account class to expose its lock object, but it seems it has to? Also wondering if there are any latent bugs I haven't spotted.

Is the getBalance() method doing the right thing to ensure memory visibility?

In getBalance(), the 'return' in the try looks ugly, but the balance field has to be read while the lock is still in place?

private ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

public void writeLock(){
    lock.writeLock().lock();
}
public void readLock(){
    lock.readLock().lock();
}
public void writeUnlock(){
    lock.writeLock().unlock();
}
public void readUnlock(){
    lock.readLock().unlock();
}

public void transferToSafe(Account b, BigDecimal amount){

    Account firstAccountToLock=null;
    Account secondAccountToLock=null;

    // Let the smaller account always get the first lock
    if (this.getAccountNo() < b.getAccountNo()){
        firstAccountToLock = this;
        secondAccountToLock = b;
    }
    else {
        firstAccountToLock = b;
        secondAccountToLock = this;
    }
    try {
        firstAccountToLock.writeLock();

        try {
            secondAccountToLock.writeLock();

            this.subtractFromBalance(amount);
            b.addToBalance(amount);

        }
        finally {
            secondAccountToLock.writeUnlock();
        }
    }
    finally {
        firstAccountToLock.writeUnlock();
    }
}

public BigDecimal getBalance(){
    try {
        this.readLock();
        return balance;
    }
    finally {
        this.readUnlock();
    }
}

Upvotes: 1

Views: 147

Answers (1)

Zim-Zam O&#39;Pootertoot
Zim-Zam O&#39;Pootertoot

Reputation: 18148

Your lock ordering looks good as well as your deadlock prevention mechanism, but two suggestions for subtractFromBalance and addToBalance to ensure correctness (and your code may already be doing this)

  1. Do your validations inside of subtractFromBalance and addToBalance, i.e. throw an IllegalArgumentException or whatever if amount is greater than the current balance for subtractFromBalance (I'm assuming that negative balances aren't allowed). You're probably doing this validation earlier, but you'll also need to do it after you've acquired the locks.

  2. Call isHeldByCurrentThread on the write locks inside of subtractFromBalance and addToBalance and throw an exception if the lock isn't held.

If you're able to withstand a bit of temporary inconsistency in your data then you can eliminate the read lock entirely: use an AtomicReference<BigDecimal> for the balance (getBalance() simply becomes return balance.get()). The inconsistency is that if you're subtracting money from AccountA and adding it to AccountB then simultaneous calls to getBalance might return the new balance for AccountA and the old balance for AccountB, however this may be safe in your system since it only ever underestimates the available funds and the accounts will eventually be consistent.

Upvotes: 1

Related Questions