Java sychronized not protecting the variable

Question

I am learning Java multithreading. This is the the code I write.

package com.company;


public class Account  {
    private double balance = 100;

    public synchronized double getBalance() {
        synchronized (this){
            return balance;
        }
    }

    public  void setBalance(double balance) {
        this.balance = balance;
    }
    public   boolean withdraw(double amount,String name){
        if(this.getBalance()>amount){
            if(Thread.currentThread().getName().equals("customer0")){
                try {
                    Thread.sleep(100);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            this.setBalance(this.getBalance() - amount);
            System.out.println(Thread.currentThread().getName() + " withdraw " + amount);
            System.out.println("Hello,  " + Thread.currentThread().getName() + " You current balance is " + this.getBalance());

            return true;
        }
        else{
            System.out.println("Sorry, " + Thread.currentThread().getName() + ". Your current balance is " + this.getBalance() + " and you  cannot withdraw " + amount);
            //System.out.println("Welcome,  " + Thread.currentThread().getName() + " Your current balance is " + this.getBalance());

            return false;
        }
    }
}

and the main class

package com.company;

import org.omg.PortableServer.THREAD_POLICY_ID;


public class Main implements Runnable {
    Account account = new Account();
    public static void main(String[] args){
            Main main = new Main();
            for(int i= 0; i< 2; i++) {
                Thread c1 = new Thread(main, "customer" + i);
                c1.start();
            }
    }

    @Override
    public void run() {
        System.out.println(Thread.currentThread().getName() + "'s balance is " + account.getBalance());
        account.withdraw(60, Thread.currentThread().getName());
        //
    }
}

I put synchronized keyword at getBalance() to make sure it can be accessible only by one thread a time. But still I get the negative balance.

customer0's balance is 100.0
customer1's balance is 100.0
customer1 withdraw 60.0
Hello,  customer1 You current balance is 40.0
customer0 withdraw 60.0
Hello,  customer0 You current balance is -20.0

What did I do wrong?

On a side note, don't use `double` for precision important values like money. — Ceiling Gecko, Jan 12 '15 at 13:51
As Juan mentioned, you should synchronize the writes instead of synchronizing the get. No need to use synchronized for getBalance — Wael, Jan 12 '15 at 13:51
@CeilingGecko, if not double then what should one use, throw some light on that. That would be of great help — User27854, Jan 12 '15 at 13:53
@ILoveCoding are you suggesting the usage of BigDecimal as it has better precision than double? or some thing else. — User27854, Jan 12 '15 at 13:59
@user2900314 `BigDecimal` has better precision than double, yes, because it does not work as a regular floating point would work (which `double` does). More info [in this question](http://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency) — Ceiling Gecko, Jan 12 '15 at 14:02

Kevvvvyp · Accepted Answer · 2015-01-12T14:05:43.420

3

I think you should be synchronizing withdraw and balance.

Your code just now could let two threads withdraw at the same time but not let them check the balance at the same time.

You only want one thread withdrawing & one thread checking the balance at one time.

(Thanks for the edit suggestions)

edited Jan 12 '15 at 14:05

answered Jan 12 '15 at 13:50

Kevvvvyp

1,704
2
18
38

2

You should synchronize both. – Mark Rotteveel Jan 12 '15 at 13:51
@MarkRotteveel - I second you. He should be synchronize on both getter and setter. – TheLostMind Jan 12 '15 at 13:54
@talex True, I hadn't look to closely at all the code. – Mark Rotteveel Jan 12 '15 at 13:54
1

@TheLostMind Withdraw needs to be synchronized as well, to guard the TOCTTOU (Time of Check To Time of Use (Update)) gap. – Mark Rotteveel Jan 12 '15 at 13:55
@MarkRotteveel - Ya. You are right. TOCTTOU is indeed critical :P. One thread could read a value and go inside `if` block and another could come and ruin it all :) – TheLostMind Jan 12 '15 at 13:59

Java sychronized not protecting the variable

1 Answers1