Java Threading - Solution to deadlock?

Question

I have put together some Java code which demonstrates deadlock in threading. On it's own, I usually get 2 lines output and an exception, sometimes before and sometimes after the output lines which is expected. The exception I get is a NullPointerException on the first line of the transfer() method.

The problem I'm having is I'd like to know how to solve this deadlock problem. I have had a search on StackOverflow of this problem and found this page:

Avoid Deadlock example

As solutions, I have tried what Will Hartung and what Dreamcash posted but I still get exceptions when trying to use synchronize or a ReentrantLock object.

Here's the code:

Account class:

public class Account {

    int id;
    double balance;

    public Account(int id, double balance){
        this.id = id;
        this.balance = balance;
    }

    public void withdraw(double amount){
        balance = balance - amount;
    }

    public void deposit(double amount){
        balance = balance + amount;
    }

    public int getID(){
        return id;
    }

    public double getBalance(){
        return balance;
    }

}

Bank Class (a singleton):

public class Bank{

    static Bank bank;

    Account a1;
    Account a2;

    private Bank(){}

    public static Bank getInstance(){
        if(bank==null){
            bank = new Bank();
            bank.setAccountOne(new Account(1, 100));
            bank.setAccountTwo(new Account(2, 100));
        }

        return bank;
    }


    public void transfer(Account from, Account to, double amount){
        from.withdraw(amount);
        to.deposit(amount);
    }


    public Account getAccountOne(){
        return a1;
    }

    public Account getAccountTwo(){
        return a2;
    }
    public void setAccountOne(Account acc){
        a1 = acc;
    }
    public void setAccountTwo(Account acc){
        a2 = acc;
    }

}

PersonOne class:

public class PersonOne implements Runnable {

    public void run() {

        Bank bank = Bank.getInstance();     

        Account a1 = bank.getAccountOne();
        Account a2 = bank.getAccountTwo();

        bank.transfer(a2, a1, 10);      

        System.out.println("T1: New balance of A1 is " + a1.getBalance());
        System.out.println("T1: New balance of A2 is " + a2.getBalance());
    }

}

PersonTwo class:

public class PersonTwo implements Runnable {

    public void run() {
        Bank bank = Bank.getInstance();
        Account a1 = bank.getAccountOne();
        Account a2 = bank.getAccountTwo();

        bank.transfer(a1, a2, 10);

        System.out.println("T2: New balance of A1 is " + a1.getBalance());
        System.out.println("T2: New balance of A2 is " + a2.getBalance());
    }

}

And finally, my main method

    public static void main(String[] args){
        PersonOne p1 = new PersonOne();
        PersonTwo p2 = new PersonTwo();

        Thread t1 = new Thread(p1);
        Thread t2 = new Thread(p2);

        t1.start();
        t2.start();
    }

Where are your implementations with the Lock usage ? Also you can look at the atomic datatypes. — Risto Novik, Dec 10 '13 at 09:31
Why would bank be a singleton? That does not make much sense and complicates your design unnecessarily... — assylias, Dec 10 '13 at 09:36
Possible duplicate of [Avoid Deadlock example](http://stackoverflow.com/questions/13326861/avoid-deadlock-example) — Bilesh Ganguly, May 21 '17 at 09:09

score 8 · Answer 1 · edited Jun 20 '20 at 09:12

The exception I get is a NullPointerException on the first line of the transfer() method.

The problem I'm having is I'd like to know how to solve this deadlock problem.

Your code cannot possible provoke any deadlocks. What it does provoke is write visibility issues: one of the threads gets to invoke the lazy Bank initializer and the other thread does not see the write.

To get deadlocks you first need any kind of locking (the synchronized keyword). Your particular NPE problem will be solved by adding synchronized to the getInstance method and it will not introduce any deadlocks.

My conclusion is that your best recourse is reading some introductory material on concurrency in Java.

+1 Good point that the code isn't close to being thread safe which is the reason for the exception. — Peter Lawrey, Dec 10 '13 at 09:38

score 2 · Answer 2 · answered Dec 10 '13 at 09:37

There is a number of solutions and some of the less obvious ones are

use one thread and don't use locks. In this example, the code will me much simpler and much faster with just one thread as the overhead of locks exceeds the work being done.
as this is just an example, you cold use just one global lock. This will not perform as well as multiple locks but it is much simpler and if you don't need the performance you should do the simpler , less likely to get bugs. This cannot get a deadlock.
if you have to use multiple lock because this is homework, you can ensure you always lock in the same order. You can do this by sorting the objects on a unique key and always lock the "first" item first.
lastly, you can lock the objects in any order by use tryLock on the second account. If this fails, release both locks and try again. You can perform a tryLock on a monitor using Unsafe.tryMonitor().

score 0 · Answer 3 · answered Dec 10 '13 at 10:19

Thanks for your answers.

I'm currently learning about threading. In the above, I was purposefully trying to create a deadlock, so that I could learn how to solve the problem of deadlocks (if I actually wanted a program that did the above, I would just avoid multi-threading).

The reason I used a singleton was so the two threads would be trying to use the same objects.

So... the above was the code I used to try to recreate a deadlock, so I was expecting problems/exceptions when running. One way I tried to then fix it was by rewriting the transfer method to look like this:

public void transfer(Account from, Account to, double amount){      
    synchronized(from){
        synchronized(to){
            from.withdraw(amount);
            to.withdraw(amount);
        }
    }
}

But I'd get a NullPointerException on the synchronized(from) line.

I also tried this in the transfer method (and account had a ReentrantLock object inside it). But this time I'd get a NullPointerException on the line that reads from.getLock().unlock()

public void transfer(Account from, Account to, double amount){      
    while(true){
        try{
            if(from.getLock().tryLock()){
                try{
                    if(to.getLock().tryLock()){
                        from.withdraw(amount);
                        to.withdraw(amount);
                        break;
                    }
                } finally {
                    to.getLock().unlock();
                }               
            }           
        } finally{
            from.getLock().unlock();
        }

        Random random = new Random();
        int i = random.nextInt(1000);
        try {
            Thread.sleep(1000 + i);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }
}

Okay... I've just tried adding synchronized to get getInstance() method, which Marko suggested, and that has solved the problem. I guess now then that I didn't have a deadlock in the first place, I had a visibility problem. — Chris, Dec 10 '13 at 10:27

Java Threading - Solution to deadlock?

3 Answers3