Why does this synchronized method not 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 ATMThread class

 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 flows, we assume that there is an initial balance in the bank account ($ 1500)

the first stream tries to withdraw $ 100 and the second stream.

I expect the final balance to be 1300, but sometimes it is 1400. Can someone explain to me why? I use synchronized methods ...

Answer 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 internal state of the account to only one thread at a time. However, your balance field is public (therefore not internal), which is the root cause of all your problems:

 public double balance = 1500; 

Using the public modifier, you get access to 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, although it looks correct with the synchronized , is not. You create two threads, and a synchronized thread is basically a lock bound to an object. This means that these two threads have separate locks, and each can access its own lock.

Think about your withDrawFromPrivateBalance() method. If you have two instances of the Account class, you can safely call this method from two threads on two different objects. However, you cannot call withDrawFromPrivateBalance() on the same object from more than one thread due to the synchronized . It looks like.

You can fix this in two ways: either use withDrawFromPrivateBalance() directly (note that synchronized no longer required here):

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

or lock the same object in both threads, unlike locking on two independent instances of the Thread object:

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

The last solution is obviously inferior to the first, because somewhere it’s easy to forget about external synchronization. Also, you should never use public fields.

Answer 2

Your private synchronized void find() method synchronizes with various locks. Try to synchronize it on the same objects as

 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 your fields private and adding a synchronized modifier to all of your recipients and setters, and then use only these methods to change the value of the fields.

Answer 3

Marking a synchronized method gets a lock on the object that runs this method, but there are two different objects: ATMThread and Account .

In any case, two different ATMThread use different locks, so their entries may overlap and conflict with each other.

Instead, you should synchronize ATMThread on the same object.