CODEWITHSUNDEEP
javamultithreadingthread-safetysynchronizedreentrantlock
pr0f3ss
pr0f3ss

Reputation: 537

Locking all instances of a class in Java

I am implementing a parallel banking system, where all operations can run concurrently. I have implemented a thread safe transferMoney method, that transfers amount from Account from to to.

transferMoney is implemented with the following code:

public boolean transferMoney(Account from, Account to, int amount) {
        if (from.getId() == to.getId()){
            return false;
        }else if(from.getId() < to.getId()) {
            synchronized(to) {
                synchronized(from) {
                    if(from.getBalance() >= amount) {
                        from.setBalance(from.getBalance()-amount);
                        to.setBalance(to.getBalance()+amount);
                    }else {
                        return false;
                    }
                }
            }
        }else {
            synchronized(from) {
                synchronized(to) {
                    if(from.getBalance() >= amount) {
                        from.setBalance(from.getBalance()-amount);
                        to.setBalance(to.getBalance()+amount);
                    }else {
                        return false;
                    }
                }
            }
        }

        return true;
    }

To prevent deadlocks, I have specified that the locks are always acquired in the same order. To assure that the locks are acquired in the same order, I am using the unique ID of Account.

Additionally, I have implemented a method that sums up the total amount of money in the bank with the following code:

public int sumAccounts(List<Account> accounts) {
    AtomicInteger sum = new AtomicInteger();

    synchronized(Account.class) {
        for (Account a : accounts) {
            sum.getAndAdd(a.getBalance());
        }
    }

    return sum.intValue();
}

Problem

When I run sumAccounts() concurrently with transferMoney(), I will end up with more (sometimes less) money in the bank before, even though no money was added. From my understanding if I lock all Account objects via synchronized(Account.class), shouldn't I get the correct sum of the bank as I am blocking the execution of transferMoney()?

What I have tried this far

I have tried the following things:

Shouldn't the lock on Account.class prevent any further transferMoney() executions? If not, how can I fix this issue?

Edit: The code for getBalance():

public int getBalance() {
        return balance;
}

Upvotes: 1

Views: 869

Answers (3)

Cratylus
Cratylus

Reputation: 54094

It is very hard to review your code because we have no way of knowing if the object accounts that you synchronize on, are the exact same instances in all the functions.
First of all, we have to agree if the sum of balances and the transfer of amounts are two operations that should be running at the same time.
I would expect that the sum of balances is the same before and after the transfer of amounts.
Additionally you are using synchronized(Account.class) in the sum of balances which is wrong. You should be synchronizing on the objects you are looping over.
Now even if you indeed are coordinating in the exact same instances you can still have the following schedule: 

Thread-1 (transfer)  
  locks from  
Thread-2 (sum balance)  
  locks first object in the list and adds the balance to the running sum and moves to next object
Thread-1  
   locks to (which is the object Thread-2) processed
   moves money from => to

You already summed the to with the amount before the increase and you could be adding from with the amount after the deduction depending on the scheduling.

The problem is that you are updating 2 objects in the transfer but only locking 1 in the sum.
What I would suggest is either:

  1. either synchronize both methods on the same lock and make them run serially
  2. set some dirty flag when the objects go in the transfer method and if that is set, skip them in the sum of balance and finish the sum when all the updates are done
  3. Why are you even doing this in Java? This should be happening in the database using transactions with ACID properties.

Upvotes: 0

Erwin Smout
Erwin Smout

Reputation: 18408

As stated in a comment, taking a lock on a class object won't take locks on all instances of that class, it will just take a lock on the Class object representing your Account class. That lock is not incompatible with locks on Account objects, so you have no synchronizing going on at all.

Taking locks on individual Account objects could be done inside your for loop (in sumAccounts) but it won't prevent schedules like this happening :

- sumAccounts locks 'first' Account and reads balance (and releases lock again at end of the synchronized block taking the lock)
- system schedules a moneyTransfer() from 'first' to 'last'
- sumAccounts locks 'last' Account and reads balance, which includes the amount that was just transferred from 'first' and was already included in the sum

So if you want to prevent that too you need to synchronize the moneyTransfer() processing on Account.class too (which then obsoletes the need for locking on the indivudual objects).

Upvotes: 1

miskender
miskender

Reputation: 7968

You can use ReadWriteLock for this case. transferMoney method will use the read lock, so it can be executed concurrently. sumAccounts method will use the write lock, so when it is executing no transferMoney(or sumAccounts) can be executed from other threads.

Using ReentrantLock and synchronizing both methods on class level, will behave the same as You have stated because they will not let concurrent execution of transferMoney method.

sample code:

final ReadWriteLock rwl = new ReentrantReadWriteLock();

public boolean transferMoney(Account from, Account to, int amount) {
  rwl.readLock().lock();
  try{
    .... Your current code here
  }
  finally {
       rwl.readLock().unlock();
  }
}

public int sumAccounts(List<Account> accounts) {
  rwl.writeLock().lock();
  try{
    // You dont need atomic integer here, because this can be executed by one thread at a time
    int sum = 0;
    for (Account a : accounts) {
        sum += a.getBalance();
    }
    return sum;
  }
  finally {
       rwl.writeLock().unlock();
  }
}

Also fair mode of Reentrant locks will tend to perform slower than non-fair modes. Check the docs for details.

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html

Upvotes: 1

Related Questions