CODEWITHSUNDEEP
javamultithreadingthread-safety
Arian
Arian

Reputation: 7719

Thread safe swapping in Java

I have a method which receives two bank account as inputs and swap their values :

Public void TransferAccount(AccountID  id1, AccountID id2){
    Account a1 = id1.GetAccount();
    Account a2 = id2.GetAccount();

    //Swap amounts.

    Temp = a1.Balance;
    a1.Balance = a2.Balance;
    a2.Balance = Temp;
}

I want to make this method to be thread-safe with the highest possible performance (I guess it means we may not make the method synchronized) , we have also to be careful about the deadlock ,

I thought of the following solution : 

Public void TransferAccount(AccountID  id1, AccountID id2){
    Account a1 = id1.GetAccount();
    Account a2 = id2.GetAccount();

//Swap amounts.
    synchronized(a1){
        wait(a2);
        synchronized(a2){
            Temp = a1.Balance;
            a1.Balance = a2.Balance;
            a2.Balance = Temp;
        }
    }
}

Is there any better implementation in terms of performance ? and by the way , is this thread-safe at all ?

Upvotes: 1

Views: 1704

Answers (1)

JB Nizet
JB Nizet

Reputation: 691655

Your code is subject to deadlock. If a thread calls swap(a2, a1) while another thread calls swap(a1, a2), you'll have a deadlock.

You must make sure that you always lock your accounts in the same order. For example, assuming all the accounts are identified by a unique ID,

public void swap(Account a1, Account a2) {
    Account first = a1;
    Account second = a2;

    if (a1.getId().compareTo(a2.getId()) > 0) {
        first = a2;
        second = a1;
    }

    synchronized (first) {
        synchronized (second) {
            // swap the balances
        }
    }
}

Another big problem is that you access the balance of an account using a public field. Public fields should almost never be used, and especially not when an object is accessed by multiple threads. Use accessor methods, and make sure they're properly synchronized, or another thread won't see the new balances after the swap. Every shared state must always be accessed in a synchronized way.

But the first thing to do with your code is to make it compile, and respect the Java naming conventions.

Upvotes: 8

Related Questions