How to handle multithreading in simple cash deposit withdraw program [closed]

Question

My instructor said to use multi-threading for update an account management system. Given below is a rough idea of the system.

Here is my source code for it.

Account class

public class Account {
    int balance= 1000;

    public int getBal(){
        return balance;
    }

    public void withdraw(int bal){
        balance= balance-bal;
    }

    public void deposit(int bal){
        balance= balance+bal;
    }
}

ThreadExercise class

public class ThreadExercise implements Runnable{

    Account acc = new Account();

    public static void main(String[] args) {
        ThreadExercise ts = new ThreadExercise();
        Thread t1 = new Thread(ts, "person 1");
        Thread t2 = new Thread(ts, "person 2");
        Thread t3 = new Thread(ts, "person 3");
        t1.start();
        t2.start();
        t3.start();
    }

    @Override
    public void run() {
        for (int i = 0; i < 3; i++) {
            makeWithdraw(100);
            if (acc.getBal() < 0) {
                System.out.println("account is overdrawn!");
            }
            deposit(200);
        }
    }


    private synchronized void makeWithdraw(int bal){
        if (acc.getBal()>=bal) {
            System.out.println(Thread.currentThread().getName()+" "+ "is try to withdraw");
            try {
                Thread.sleep(100);
            } catch (Exception e) {
                e.printStackTrace();
            }
            acc.withdraw(bal);
            System.out.println(Thread.currentThread().getName()+" "+ "is complete the withdraw");
        }else{        
            System.out.println(Thread.currentThread().getName()+ " "+"doesn't have enough money for withdraw ");
        }
    }

    private synchronized void deposit(int bal){
        if (bal>0) {
            System.out.println(Thread.currentThread().getName()+" "+ " is try to deposit");
            try {
                Thread.sleep(100);
            } catch (Exception e) {
                e.printStackTrace();
            }
            acc.deposit(bal);
            System.out.println(Thread.currentThread().getName()+" "+ "is complete the deposit");
        }else{        
            System.out.println(Thread.currentThread().getName()+ " "+"doesn't have enough money for deposit");
        }
    }
}

Code is working fine. But I really think something is missing this code. Can you please help me for finding that fault.

Is it not be enough synchronizing the makeWithdraw() and deposit() methods in ThreadExercise class and should I remove that synchronizing and synchronize the withdraw() and deposit() in Account class. Please give me a clear idea.
Thank you for your support.

Hurricane

What do you feel is missing in this code? – Mediocrity

Here instead of using synchronized method you can use use synchronized block. As using synchronized block you can reduce the content which is of your interest (means which actually required lock) in your case only two line (acc.deposite(bal) & acc.withdraw(bal)). – Sheers

@CeilingGecko are asking about wait and notify mechanism ? – Marlie

Withing the run method I put makeWithdraw() and deposit() method. It is just so robotic action. Nothing logical. @Ceiling Gecko can please give me an idea. – Hurricane

@BhargavModi where should I put wait and notify. :( – Hurricane

@kanti you mean include acc.deposite(bal) & acc.withdraw(bal) to a synchronized block inside the run method? right?. – Hurricane

Answer 1

Account class

public class Account {
public static Account account;
private static int balance = 1000;
private static Person person;

private Account() {
}

public static Account getAccount(Person p) {
    if (account == null) {
        account = new Account();
    }
    Account.person = p;
    return account;
}

public static int getBal() {
    return balance;
}

public synchronized void withdraw(int bal) {
    try {

        if (balance >= bal) {
            System.out.println(person.getName() + " " + "is try to withdraw");
            try {
                Thread.sleep(100);
            } catch (Exception e) {
                e.printStackTrace();
            }
            balance = balance - bal;
            System.out.println(person.getName() + " " + "is complete the withdraw");
        } else {
            System.out.println(person.getName() + " " + "doesn't have enough money for withdraw ");
        }
        System.out.println(person.getName() + " " + " withdraw Rs." + balance);
    } catch (Exception e) {
        e.printStackTrace();
    }
}

public synchronized void deposit(int bal) {
    try {
        if (bal > 0) {
            System.out.println(person.getName() + " " + " is try to deposit");
            try {
                Thread.sleep(100);
            } catch (Exception e) {
                e.printStackTrace();
            }
            balance = balance + bal;
            System.out.println(person.getName() + " " + "is complete the deposit");
        } else {
            System.out.println(person.getName() + " " + "doesn't have enough money for deposit");
        }
        System.out.println(person.getName() + " " + " deposit Rs." + balance);
    } catch (Exception e) {
        e.printStackTrace();
    }
}}

Person class

public class Person {
private String name;

public Person(String name) {
    this.name = name;
}

public String getName() {
    return name;
}

public void setName(String name) {
    this.name = name;
}

@Override
public String toString() {
    return name;
}}

ThreadExercise class

public class ThreadExercise extends Thread implements Runnable {

private Person person;

public ThreadExercise(Person p) {
    this.person = p;
}

public static void main(String[] args) {

    ThreadExercise ts1 = new ThreadExercise(new Person("person 1"));
    ts1.start();
    ThreadExercise ts2 = new ThreadExercise(new Person("person 2"));
    ts2.start();
    ThreadExercise ts3 = new ThreadExercise(new Person("person 3"));
    ts3.start();

}

@Override
public void run() {
    for (int i = 0; i < 3; i++) {
        try {
            Account acc = Account.getAccount(person);
            acc.withdraw(100);
            try {
                Thread.sleep(200);
            } catch (InterruptedException ex) {
                Logger.getLogger(ThreadExercise.class.getName()).log(Level.SEVERE, null, ex);
            }
            if (acc.getBal() < 0) {
                System.out.println("account is overdrawn!");
            }
            acc.deposit(200);

        } catch (Exception e) {
            e.printStackTrace();
        }
    }
    System.out.println("Final Acc balance is Rs." + Account.getBal());
}}

Overbearing

Answer 2

You Account class is not thread safe. Although you have synchronized the Deposit & withdraw methods of ThreadExercise class, the underlying balance can be changed while the deposit / withdraw has locked the thread.

Consider Scenario

Thread 1 calls ThreadExercise.deposit it checks the balance and wait. The same time Thread 2 wakes up and update the balance.

So you account balance is not really synchronized against concurrent deposit + withdraw calls.

You can define the balance as below.

 AtomicInteger balance = new AtomicInteger(1000);

Then the withdraw method can be written as below

public boolean withdraw (int amtToWithdraw, int existingBalance){
    return balance.compareAndSet(existingBalance,existingBalance-amtToWithdraw); 
}

public void deposit(int amtToDeposit, int existingBalance){
    return balance.compareAndSet(existingBalance,existingBalance+amtToDeposit);
}

You may need to handle the failure scenario.

Plumcot

Thank you. So any idea to change that ? – Hurricane

Consider using AtomicInteger.compareAndSet for balance – Plumcot

So as your answer I'm returning 2 parameters " existingBalance,existingBalance-amtToWithdraw " but I really don't need a existingBalance here – Hurricane

Answer 3

Considering the design the Account class must be synchronized (the methods in it).

The way it currently is someone else may retrieve an instance to an account and use it in a manner which is not thread-safe. In this case simply invoking the Account'methods from somewhere else would beak it.

public class Account {
  private final Object lock = new Object();
  // Must be private to be thread-safe!
  private int balance= 1000;

  public int getBal(){
    return balance;
  }

  public synchronized void withdraw(int bal){
    synchronized (lock) {
      balance= balance-bal;
    }
  }

  public synchronized void deposit(int bal){
    synchronized (lock) {
      balance= balance+bal;
    }
  }
}

Placate

You mean synchronized all account methods or just withdraw() and deposit – Hurricane

if I made withdraw() and deposit() method synchronized shouldn't I have to make them private ? ? Then it'll be a issue. – Hurricane

Multi-threading is about mutable state -> making only the methods thread-safe which cause the object to mutate would be fine. Basically there is no need to make getBalance() thread-safe. Thread-safety and visibility do not interfere this much. Hence there is no need to make synchronized methods private. However letting references on mutable objects / locks to escape somewhere can cause deadlocks (by improper use of API). To avoid this create a lock such as private final Object lock = new Object(); and synchronize using that lock, see code example in next comment. – Placate

public final class Account { private final Object lock = new Object(); // no thread-safety if not private private int balance= 1000; public int getBal(){ return balance; } public void withdraw(int bal){ synchronized (lock) { balance= balance-bal; } } public void deposit(int bal){ synchronized (lock) { balance= balance+bal; } } } – Placate

So should I remove synchronized in ThreadExcercise class's makeWidthdraw() and deposit() methods? – Hurricane

@Placate that is incorrect. If the getBalance() is not synchronized, a thread might still see an old value for the balance, although it has been updated one or several times by another thread. all the accesses to the mutable state must be synchronized. – Stichous

@JB Nizet: I saw that. And you are right, a thread may see an old value. However it should never see a balance which never existed as the assignements are atomic. However notice that the Java Language Specification not requires that changing of long values is atomic (on 32-bit it might be non-atomic). Hence in case of long values or more complex computations which are not atomic the getBalance must be synchronized too. Seeing an old value may only be a problem if invoking more code depending on the value returned by getBalance which may invoke some of the synchronized methods or similar stuff. – Placate

@Placate can you please add a code to your answer clearing what you are saying. I'm not having a good idea about the "private final Object lock = new Object();" source code. Please I'm not very much clear with locks – Hurricane

At the moment I'm not completely sure what object is used for the lock if you have a method public synchronized ... . If a class is used no two instances of this class can be concurrently modified. If the object (for example the Account instance) is used as lock someone else may write code, created an instance a of Account, writing a synchronized method synchronizing on a. This way a deadlock can be created. Thus to reduce thread contention / bottlenecks and to avoid such problems I mostly prefer to have such lock objects. However using an AtomicInteger would also be fine in this case. – Placate

Answer 4

I'm not sure the other answers have been clear.

You've synchronized the methods on the ThreadExercise class. That means only one thread can invoke those methods on a given ThreadExercise object at once. That has no effect because each thread object will only invoke methods on one such object anyway.

You need to synchronize the methods of the Account class to make sure only one thread is invoking one method on any given Account at a time.

Of course in any real system Account objects would be (somehow) serialized to some database or object store and you would need to make sure that your application didn't introduce two 'doppelganger' objects that represent one 'physical' account. That might be tricky on a distributed system with multiple ATM Switches.

If you introduce the idea of a balance transfer you might need to introduce further synchronization. That's particularly true if it was unacceptable for some observer process to see:

Account 1: $100
Account 2: $0

Account 1: $40
Account 2: $0

Account 1: $40
Account 2: $60

In which a $60 transfer is seen to disappear and re-appear. There's no business problem there. Banks make millions taking money from X sitting on it and then passing it on to Y for no good reason than they can milk their clients. I'm just making the point that adding synchronized to methods isn't the whole answer to concurrent programming.

I did once see an organization that managed to execute such a transfer and have an error in the middle and leave the accounts in the state:

Account 1: $100
Account 2: $60

Where a $60 ($10 millions IRL) from Account 1 to 2 arrived but never left! That's another story...

However to answer the question:

public class Account {
    int balance= 1000;

    public int getBal(){
        return balance;
    }

    public synchronized void withdraw(int bal){
        balance= balance-bal;
    }

    public synchronized void deposit(int bal){
        balance= balance+bal;
    }
}

I have provocatively not synchronized getBal(). On the one hand int reads and writes are atomic so it will always read a consistent value of balance and not some 'bastard' where (say) a write operation has only updated the low bytes. If you changed it to long the JVM doesn't make that guarantee anymore.

However not synchronizing it means you could see that anomalous position:

  Account 1: $100
  Account 2: $60

The could occur even if your code was:

account1.withdraw(60);
account2.deposit(60);

That's because synchronization doesn't just introduce blocking but also effects a memory barrier. Suppose a separate thread had account1 cached but not account2 without synchronization it wouldn't know account1 was stale but fetch an up to date version of account2.

It's worth a footnote that your class is so simple you could get away with using java.util.concurrent.atomic.AtomicInteger using addAndGet(int delta). However as soon as you start adding a sophistication (such as an overdraft limit) you'll need to go back to synchronization.

Succory

you answer seems to so interesting. I'm also worrying the real scenario here. What if this is a real ATM system? As you say I can put synchronized block around the account class's withdraw and deposit. What if this is a system 1000 of clients are accessing. Should I use a thread pool here? or should I generate a new thread all the time a client click the deposit or withdraw button. Please clear your idea with a source code. I really love to see that. – Hurricane

@Succory I do agree with your solution, basically in lemon term, we do lock the object on thread per transition and by this way consistency keeps maintains. I want to understand how does it work in case of application deployed on multiple vm's. so does the object get lock for same thread on all the vm's?? please clarify.. – Encampment