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.

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());
}}

Answer 2

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;
    }
  }
}

Answer 3

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.