1
\$\begingroup\$

I have made a class which is a thread-safe implementation supporting Deposit, Withdrawal, Check Balance Querying and Transferring of money. I have a class Account:

public class Account {
    private int balance;
    private String address;
    private String userId;
    final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
    Lock transferLock = new ReentrantLock();
    public Account(String userId){
        this.userId = userId;
    }

    public void add(int amount){
        balance += amount;
    }
    public void sub(int amount){
        this.balance -= amount;
    }
    public int getBalance(){
        return balance;
    }
}

I use a ReentrantReadWriteLock for Depositing and Withdrawing which acquires a Write Lock. Read lock is acquired when checking an Account's balance. A 'transferLock' is used to lock the transferring of funds from Account A to Account B.

Here are the main functions:

 public boolean deposit(String userId, int amount){
        Account account = getUserAccount(userId);
        Lock accountWriteLock = account.lock.writeLock();
        accountWriteLock.lock();

        account.add(amount);

        accountWriteLock.unlock();
        return true;
    }

    public boolean withdraw(String userId, int amount){
        Account account = getUserAccount(userId);
        Lock accountWriteLock = account.lock.writeLock();
        accountWriteLock.lock();

        if(account.getBalance() < amount){
accountWriteLock.unlock();
            return false;
        }
        account.sub(amount);

        accountWriteLock.unlock();
        return true;
    }

    public int checkBalance(String userId){
        Account account = getUserAccount(userId);
        Lock accountReadLock = account.lock.readLock();
        accountReadLock.lock();

        int balance = account.getBalance();

        accountReadLock.unlock();
        return balance;
    }


    public boolean transfer(String fromAccountUserId, int amount, String toAccountUserId) throws Exception {
        Account fromAccount = getUserAccount(fromAccountUserId);
        Account toAccount = getUserAccount(toAccountUserId);
        Random rand = new Random();
        Lock fromAccountTransferLock = fromAccount.transferLock;
        Lock toAccountTransferLock = toAccount.transferLock;
        while (true) {
            if (fromAccountTransferLock.tryLock()) {
                fromAccount.lock.writeLock();
                if (toAccountTransferLock.tryLock()) {
                    try {
                        withdraw(fromAccountUserId, amount);
                        deposit(toAccountUserId, amount);
                        break;
                    } catch (Exception e) {
                        return false;
                    } finally {
                        toAccountTransferLock.unlock();
                    }
                }
                fromAccountTransferLock.unlock();
                Thread.sleep(rand.nextInt(1001));
            }
        }
        return true;
    }

Assume that getUserAccount(String userId) retrieves the appropriate Account object.

What I Would Like Reviewed
1. Is my code thread-safe?
2. Any improvements you think would be good?

Notes
1. I prevent Deadlock in Transferring from Account A to Account B by allowing the holding thread to release its lock.
2. I prevent LiveLock in transfer(...) by sleeping the thread for a random number of milliseconds between [0,1000] ms.

EDIT: Here is the getUserAccount() code:

public Account getUserAccount(String userId){
        return new Account(userId);
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Hey Jim, could you add the full class where the method getUserAccount is defined, for example. Otherwise without the full code it's difficult to provide a good review :) \$\endgroup\$ – IEatBagels Nov 23 '19 at 1:00
  • \$\begingroup\$ Hi IEatBagels, thanks for the reply. I have added the getUserAccount() function. As you can see right now it just returns a new Account() object. I am just testing the other functions first. I hope this is okay for you to still review. I appreciate the help :) Currently I am not communicating with any kind of database. \$\endgroup\$ – Jim Nov 23 '19 at 11:21
0
\$\begingroup\$

Let's break this down.

  1. You have two locks for single object that guarding same fields.

It's generally bad practice, and it doesn't make sense as second lock(transferLock) is used only in single operation. It won't guard for other operations.

It could possibly work if you have a static read-only lock for transfer with the following semantics: add/sub/get are global read lock and transfer is global write lock, then yu should acquire read lock on transfer on regular operations and write-lock on transfer op, but that would allow on;y single transfer at a time, and that's definitely not what you are trying to achieve.

  1. Your locks leak outside of your main class (reed aren't private/protected).

This could lead to uncontrolled use from outside, say future versions of code could misuse locks. I can see that they have package level visibility, that's semi ok, but Account#add,Accound#sub, Account#getBallance are public and not guarded by locks, so that destroys the purpose of haveing locks package-private.

I'd suggest put all usage of locks inside instance methods mentioned above, and add another method:

class Account{
public void transfer(Account other, int amount) { ... }
}

Then all logic related to locking would be in a single, controlled place.

  1. Lock ordering. If you are using granular locks, so no single lock for all accounts and you need to lock several objects you should have STRICT lock order.

You are trying to achieve this with tryLock and while loop, this COULD actually work if you have used your rw lock, but it would be possible to deadlock as you are acquiring other locks then. And it's not really performant to wait in spin-loop here, you can wait forever.

I'd suggest to user your rw-lock but first, sort two accounts by accountId, then take write locks in order of account ids from lower to higher.

//pseudocode
if (acc1.id<acc2.id) {
  acc1.lock.writeLock().lock()
  acc2.lock.writeLock().lock()
}else{
  acc2.lock.writeLock().lock()
  acc1.lock.writeLock().lock()
}

It could be actually combined with tryLock, but it could take a while to acquire such locks.

  1. Minor thing, it's preferable to call unlock() inside finally block, so no objects would stick in a locked state.
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.