How to avoid LOCK_INVERSION or LOCK_ORDER problem in two independent class calling each other function?

Question

I have two independent classes and both classes functions are calling each other. I have put the synchronization block on both classes functions but coverity is giving the error regarding LOCK_INVERSION and LOCK_ORDER or it might result in deadlock, not sure.

I am getting below error in coverity on my production code.

Acquiring lock 'lockname' conflicts with the lock order established elsewhere.

Sample example:

package multithreading;

public class Test1 implements Runnable {
    private static Test1 instance = null;

    public static synchronized Test1 getInstance() {
        if (instance == null) {
            instance = new Test1();
            return instance;
        } else {
            return instance;
        }
    }

    private Object lock = new Object();

    public void sendUpdate() {
        synchronized (lock) {
            try {
                Thread.sleep(3000l);
                System.out.println(getClass().getSimpleName() + "sendUpdate");
                Test2.getInstance().send();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

        }
    }
    
    public void sendUpdate1() {
        synchronized (lock) {
            try {
                Thread.sleep(3000l);
                System.out.println(getClass().getSimpleName() + "sendUpdate1");
                Test2.getInstance().send();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

        }
    }


    public static void main(String[] args) {
        Thread t = new Thread(Test1.getInstance());
        t.start();
    }

    @Override
    public void run() {
        while (true) {
                this.sendUpdate();

        }
    }

}

package multithreading;

public class Test2 implements Runnable {
    private static Object object = new Object();
    private static Test2 instance = null;

    public static synchronized Test2 getInstance() {
        if (instance == null) {
            instance = new Test2();
            return instance;
        } else {
            return instance;
        }
    }
    
    public void send1() {
        synchronized (object) {
            try {
                Thread.sleep(3000l);
                System.out.println(getClass().getSimpleName() + "send1");
                Test1.getInstance().sendUpdate();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    public void send() {
        synchronized (object) {
            try {
                Thread.sleep(3000l);
                System.out.println(getClass().getSimpleName() + "send");
                Test1.getInstance().sendUpdate();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    public static void main(String[] args) {
        Thread t = new Thread(Test2.getInstance());
        t.start();
    }

    @Override
    public void run() {
        while (true) {
            Test2.getInstance().send1();
        }

    }
}

Analysis summary report:
------------------------
Files analyzed                 : 2 Total
    Java (without build)       : 2
Total LoC input to cov-analyze : 90
Functions analyzed             : 15
Paths analyzed                 : 71
Time taken by analysis         : 00:00:18
Defect occurrences found       : 1 LOCK_INVERSION

I think this can lead to deadlock problem but would like to understand how to basically handle synchronization in two independent classes calling each other functions.

How can we maintain the lock order among two classes? Should I use the ENUM based shared lock but I am little bit worried about the performance or there is any other way to handle such scenario?

Answer 1

The reason for the Coverity report is the code contains a potential deadlock error. Specifically:

• Test1.sendUpdate() acquires Test1.lock, then calls Test2.send(), which acquires Test2.object. Meanwhile,
• Test2.send() acquires Test2.object, then calls Test1.sendUpdate(), which acquires Test1.lock.

If thread T1 executes Test1.sendUpdate() at the same time as another thread T2 executes Test2.send(), this ordering is possible:

• T1 acquires Test1.lock.
• T2 acquires Test2.object.
• T2 attempts to acquire Test1.lock, and blocks because it is already held by T1.
• T1 attempts to acquire Test2.object, and blocks because it is already held by T2.
• Neither thread can make progress; this is a deadlock.

When a single thread must hold more than one lock simultaneously, the typical way to prevent deadlock is to ensure that the order in which locks are acquired is always consistent. Here, that means either Test1.lock is always acquired first, or else Test2.object is always acquired first.

One way to do this in your code is to change Test2.send() so it acquires Test1.lock first:

public void send() {                 // in class Test2
    Test1 test1 = Test1.getInstance();
    synchronized (test1.lock) {      // Test1.lock first  <-- ADDED
        synchronized (object) {      // Test2.object second
            try {
                ...                  // same code as before
                test1.sendUpdate();
                ...
            }
        }
    }
}

Inside send(), the call to sendUpdate() will acquire Test1.lock a second time. That is ok because locks in Java are recursive.

The above code requires that Test1.lock be public, and is probably the simplest fix in this case. It's of course easy to expose a getter instead if the public data member is a concern.

Alternatively, you could replace the existing pair of locks with a single lock that protects both classes. That would be safe from deadlocks, but could reduce the opportunities for concurrent execution. Whether that will significantly affect application performance is impossible to say without knowing more about what it's really doing (which could be the topic of a new question).