why doesn't this synchronized method work as expected?

Question

I have a class called "Account"

public class Account {

    public double balance = 1500;

    public synchronized double withDrawFromPrivateBalance(double a) {
        balance -= a;
        return balance;
    }
}

and a class called ATMThread

public class ATMThread extends Thread {
    double localBalance = 0;
    Account myTargetAccount;

    public ATMThread(Account a) {
        this.myTargetAccount = a;
    }

    public void run() {
        find();
    }

    private synchronized void find() {
        localBalance = myTargetAccount.balance;
        System.out.println(getName() + ": local balance = " + localBalance);
        localBalance -= 100;
        myTargetAccount.balance =  localBalance;
    }

    public static void main(String[] args) {
        Account account = new Account();
        System.out.println("START: Account balance = " + account.balance);

        ATMThread a = new ATMThread(account);
        ATMThread b = new ATMThread(account);

        a.start();
        b.start();

        try {
            a.join();
            b.join();
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        }

        System.out.println("END: Account balance = " + account.balance);
    }

}

I create two threads, we assume there is an initial balance in the bank account(1500$)

the first thread tries to withdraw 100$ and the second thread as well.

I expect the final balance to be 1300, however it is sometimes 1400. Can someone explain me why? I'm using synchronized methods...

Solution 1

This method is correct and should be used:

public synchronized double withDrawFromPrivateBalance(double a)
{
          balance -= a;
          return balance;
}

It correctly restricts access to the account internal state to only one thread at a time. However your balance field is public (so not really internal), which is the root cause of all your problems:

public double balance = 1500;

Taking advantage of public modifier you are accessing it from two threads:

private synchronized void find(){
    localBalance = myTargetAccount.balance;
    System.out.println(getName() + ": local balance = " + localBalance);
    localBalance -= 100;
    myTargetAccount.balance =  localBalance;
}

This method, even though looks correct with synchronized keyword, it is not. You are creating two threads and synchronized thread is basically a lock tied to an object. This means these two threads have separate locks and each can access its own lock.

Think about your withDrawFromPrivateBalance() method. If you have two instances of Account class it is safe to call that method from two threads on two different objects. However you cannot call withDrawFromPrivateBalance() on the same object from more than one thread due to synchronized keyword. This is sort-of similar.

You can fix it in two ways: either use withDrawFromPrivateBalance() directly (note that synchronized is no longer needed here):

private void find(){
    myTargetAccount.withDrawFromPrivateBalance(100);
}

or lock on the same object in both threads as opposed to locking on two independent Thread object instances:

private void find(){
    synchronized(myTargetAccount) {
      localBalance = myTargetAccount.balance;
      System.out.println(getName() + ": local balance = " + localBalance);
      localBalance -= 100;
      myTargetAccount.balance =  localBalance;
    }
}

The latter solution is obviously inferior to the former one because it is easy to forget about external synchronization somewhere. Also you should never use public fields.

Solution 2

Your private synchronized void find() method is synchronizing on different locks. Try synchronizing it on same objects like

private void find(){
    synchronized(myTargetAccount){
        localBalance = myTargetAccount.balance;
        System.out.println(getName() + ": local balance = " + localBalance);
        localBalance -= 100;
        myTargetAccount.balance =  localBalance;
    }
}

You can also make your Account class more thread safe, by making its fields private and adding synchronized modifier to all its getters and setters and then use only this methods to change value of fields.

Solution 3

Marking a method synchronized obtains a lock on the object that the method is being run on, but there are two different objects here: the ATMThread and the Account.

In any event, the two different ATMThreads are using different locks, so their writes can overlap and conflict with one another.

Instead, you should have both ATMThreads synchronize on the same object.