user5415672
user5415672

Reputation: 85

How to resolve this deadlock?

I have added comments in code to explain from where deadlock is occurring. Basically, There are two threads. Each thread acquires lock on an Manager object and then go for acquiring lock on static resource, which is a map of all the Manager objects in the application.Both thread calls get() on map. Manager class has overridden equals() method. equals() further calls some synchronized method of Manager class. So a get() on map will need object level lock on each object in the map one by one until key matches because equals is overridden. I can only change the code in sub classes(Sub1 and Sub2) and avoid the deadlock, as I don't have access to other classes.

Edit: I don't have access to syncMap. The code in 'synchronized' block executes in third party code whose API I call to.

Can I avoid this by acquiring lock in finally on Manager, rather than before try block ?!

    public class Parent{
        protected Manager manager;
    }

    public class Global{
        private static final Map syncMap = Collections.synchronizedMap(new HashMap());
        //syncMap contains all the objects of Manager in the application
    }

    class Manager{


        public boolean equals(Object o){

            Manager obj = (Manager)o;
            return obj.getURL().equals(getURL());
        }

        public final synchronized String getURL(){
            return msettings.getDBURL(); //msettings is a global variable
        }


    }


    //Thread-1 is executing someMethod() of this class

    class Sub1 extends Parent{
        Global global;
        //consider manager and Global object are not null
        public void someMethod()
        {
            synchronized(manager){// Thread-1 succesfully takes object level lock on a manager object, say Manager01
                try{
                    global.syncMap.get(manager);
                    // Thread-1 Succesfully takes class level lock on syncMap
                    //  get() calls equals() for each object in syncMap. 
                    //equals() need object lock on each Manager Object in map as it further calls synchronized getURL()
                    // But on one manager Object(Manager02) Thread-2 has already acquired lock and is waiting for lock on syncMap which this thread-1 holds

                }
                finally{
                    manager.releaseConnection();
                }

            }
        }
    }

    //Thread-2 is executing otherMethod() of this class
    class Sub2 extends Parent{ 
        public void otherMethod()
        {
            synchronized(manager){// this takes a lock on manager(Manager02)
                try{
                    global.syncMap.get(manager);
                    // this is blocked as syncMap is aquired by thread-1


                }
                finally{
                    manager.releaseConnection();
                }

            }
        } 
    }

Upvotes: 2

Views: 529

Answers (2)

gustf
gustf

Reputation: 2017

First of all, you really should try to eliminate the need for synchronization in the equals method. It will cause more trouble than it solves so if a redesign is possible then I think thats the best way.

However, if you restructure the code a bit and move the global.syncMap.get(manager) to before the synchronization block it would not generate a deadlock

public Class Parent{
    protected Manager manager;
}

class Global{
    private static final Map syncMap = Collections.synchronizedMap(new HashMap());
    //syncMap contains all the objects of Manager in the application
}

class Manager{


    public boolean equals(Object o){

        Manager obj = (Manager)o;
        return obj.getURL().equals(getURL());
    }

    public final synchronized String getURL(){
        return msettings.getDBURL(); //msettings is a global variable
    }


}


//Thread-1 is executing someMethod() of this class

class Sub1 extends Parent{
    Global global;
    //consider manager and Global object are not null
    public void someMethod()
    {
         try {
             global.syncMap.get(manager);
             synchronized(manager){

             }
         }
         finally{
             manager.releaseConnection();
         }            
    }
}

//Thread-2 is executing otherMethod() of this class
class Sub2 extends Parent{ 
    public void otherMethod()
    {
         try {
             global.syncMap.get(manager);
             synchronized(manager){

             }
         }
         finally{
             manager.releaseConnection();
         }                        
    } 
}

UPDATE Alternative synchronization over Global.class, could probably use instance variable global also instead of Global.class

UPDATE Changed synchronization to be over Manager.class instead of Global.class.

class Sub1 extends Parent
{
    Global global;

    public void someMethod()
    {
        synchronized (Manager.class) { 
            try {
                global.syncMap.get(manager);
            }
            finally {
                manager.releaseConnection();
            }
        }
    }
}


class Sub2 extends Parent
{
    Global global;

    public void otherMethod()
    {
        synchronized (Manager.class) { 
            try {
                global.syncMap.get(manager);
            }
            finally {
                manager.releaseConnection();
            }
        }
    }
}

Upvotes: 1

nukie
nukie

Reputation: 681

After new portion of information I don't see another solution except of turning all processing into serial-style. So you can put all manager-associated API calls in one synchronized method of some wrapping class and use this wrapper as a single entry-point for third-party API.

class BrutalWrapper {
    public synchronized void doIt(Manager manager)
    {
        try{
            global.syncMap.get(manager);

        }
        finally{
            manager.releaseConnection();
        }
    }
}

class Sub1 extends Parent{
    BrutalWrapper brutal;
    public void someMethod()
    {
        brutal.doIt(manager);
    }
}

class Sub2 extends Parent{
    BrutalWrapper brutal;
    public void someMethod()
    {
        brutal.doIt(manager);
    }
}

Upvotes: 1

Related Questions