Reputation: 85
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
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
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