why doesn't this synchronized method work as expected?

java, multithreading, synchronized

Solution

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.

Problem

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...

Original source