Changes in Java 6 synchronization?

Question

I am looking at some code that is causing an issue (Deadlock) in Java 6 and above, but not in Java 1.5.

BMP Bean:

private MyClass m_c;
public String ejbCreate(String id) throws CreateException, MyException
{
    try
    {
        m_c = Singleton.getInstance().getObj(id);
    }
    catch (MyException e)
    {
        synchronized (Singleton.getInstance())
        {
            //check again
            if (!Singleton.getInstance().hasObj(id)) {
                m_c = new MyClass(id);
                Singleton.getInstance().addObj(id, m_c);
            }
            else {
                m_c = Singleton.getInstance().getObj(id);
            }
        }
    }
}

Singleton:

private Map objCache = new HashMap();
private static Singleton INSTANCE = new Singleton();
public static Singleton getInstance() {
    return INSTANCE;
}
public void addObj(String id, MyClass o)
{
    if (this.objCache.containsKey(id)) {
        this.objCache.remove(id);
    }
    this.objCache.put(id, o);
}

public MyClass getObj(String id) throws Exception
{
    MyClass o = null;
    o = (MyClass)this.objCache.get(id);
    if (o == null) {
        throw new MyException("Obj " +id+ " not found in cache");
    }
    return o;
}

public boolean hasObj(String id)
{
    return this.objCache.containsKey(id);
}

The empirical evidence so far shows that putting synchronization round the whole try/catch resolves the deadlock when using Java 6.

Clearly there can be one or more threads calling

Singleton.getInstance().getObj(id) 

without obtaining the lock whilst another thread has the lock and is executing the code in the synchronized block, but even after considering memory synchronization detailed in JSR-133, it doesn't look like there should be any issues in this scenario.

I am aware that I haven't explained what the issue is apart from saying it is a deadlock and that it is not ideal to paint only a prat of the picture but to paint the whole picture would take a very big canvas.

I have looked at the notes for Java 6 release and the only area that sounds relevant is around uncontended synchronization, but I do not know if that is significant in this case.

Thank you for any help.

Answer 1

I suspect you are not getting a deadlock (holding two locks in two different threads obtained in a different order), but rather going into an infinite loop. This can happen with HashMap if you are accessing it in a manner which is not thread safe. What happens in the linked list used to handle collisions appears to go back on itself and the reader runs forever. This has always been an issue, though some subtle difference in Java 6 could show up this problem when a different version might not.

I suggest you fix this class so it uses a thread safe collection and not retry on Exception because there is not guarantee this will happen.

There is a lot you could do to improve this class but what you really need is ConcurrentMap.computeIfAbsent added in Java 8.

Note: there is no reason to

• check a key exists before attempting to remove it.
• remove a key just before attempting to put it.
• throw an Exception instead of returning null.
• returning null when you can pass it a factory. (as per computeIfAbsent)
• use a factory when the type is known in advance.

I suggest you

• use a ConcurrentMap for thread safe concurrent access.
• use an enum for a Singleton.

Both of these were added in Java 5.0.

public enum MyClassCache {
    INSTANCE;

    private final Map<String, MyClass> cache = new ConcurrentHashMap<>();

    public boolean hasId(String id) {
        return cache.containsKey(id);
    }

    public MyClass get(String id) throws IllegalStateException {
        MyClass ret = cache.get(id);
        if (ret == null) throw new IllegalStateException(id);
        return ret;
    }

    public MyClass getOrCreate(String id) throws IllegalStateException {
        MyClass ret = cache.get(id);
        if (ret == null) {
            synchronized (cache) {
                ret = cache.get(id);
                if (ret == null) {
                    cache.put(id, ret = new MyClass(id));
                }
            }
        }
        return ret;
    }
}

In Java 8 you can use computeIfAbsent

public MyClass getOrCreate(String id)  {
    return cache.computeIfAbsent(id, MyClass::new);
}

Answer 2

Am I right that the core of this question is the difference between:

public void ejbCreate1(String id) throws Exception {
    try {
        m_c = Singleton.getInstance().getObj(id);
    } catch (Exception e) {
        synchronized (Singleton.getInstance()) {
            //check again
            if (!Singleton.getInstance().hasObj(id)) {
                m_c = new MyClass(id);
                Singleton.getInstance().addObj(id, m_c);
            } else {
                m_c = Singleton.getInstance().getObj(id);
            }
        }
    }
}

and

public void ejbCreate2(String id) throws Exception {
    synchronized (Singleton.getInstance()) {
        try {
            m_c = Singleton.getInstance().getObj(id);
        } catch (Exception e) {
            //check again
            if (!Singleton.getInstance().hasObj(id)) {
                m_c = new MyClass(id);
                Singleton.getInstance().addObj(id, m_c);
            } else {
                m_c = Singleton.getInstance().getObj(id);
            }
        }
    }
}

in Java-6 that can cause the first to hang and the second to work fine.

Clearly the primary difference is that getObj might be called by two different threads at the same time, and may even be called while another threads is creating the new object.

From Is it safe to get values from a java.util.HashMap from multiple threads (no modification)? it is likely that you are not in that situation. Conclusion is that one thread is readng from the Map (perhaps o = (MyClass) this.objCache.get(id);) while another is writing to the map by calling addObj. This is clearly a recipe for the read to crash and burn.

See Is a HashMap thread-safe for different keys? for details about the potential sinkholes.