Java Synchronization: Changing the lock-reference

[flythefriendlyskies]

Hi,

we have a problem with our application standing still for up to several
minutes in a method that looks like the method get() below. The most
obvious reason would be that there is one thread locking on 'value' and
all other threads simply wait on the first one. The issue is: With all
traces we have so far we still have no evidence for a thread really
being inside the synchronized block for quite a while.
Most of the time everything runs well, 99% of the application runtime
everything is fine, but the pause times (up to three minutes within a
web app) are simply too high to leave the system as it is right now.

So, here is the code (written a long time ago...):
Firstly, it reminds of the double-checked locking problem, but I don't
think that this is an issue here. Secondly, within the synchronization
block there is a new assignment to the variable that is used for
locking. I've never seen this pattern before. Are you aware of any
potential problems here? From the code, do you think there are other
reasons why threads would spend >30 seconds in the get() method?
We run on IBM JVM 1.4.2

public Object get() {
Object value = getInternal();
if (value instanceof String) {
return value;
}

synchronized (value) {
value = dcl.get(KEY);
if (value instanceof String) {
return value;
}

dcl.put(KEY, VALUE);
}
return VALUE;
}
public Object getInternal() {
Object result = dcl.get(KEY);
if (result == null) {
result = new Object();
dcl.put(KEY, result);
}
return result;
}



Thanks for your help,
Michael

 

[Thomas Hawtin]

We run on IBM JVM 1.4.2

For concurrency work you might try the backport of java.util.concurrent.
Or much better upgrade to 1.5 - it is much better tested than 1.4 and
the Sun version was released over two years ago.

public Object get() {
Object value = getInternal();
if (value instanceof String) {
return value;
}

synchronized (value) {
value = dcl.get(KEY);
if (value instanceof String) {
return value;
}

dcl.put(KEY, VALUE);
}
return VALUE;
}
public Object getInternal() {
Object result = dcl.get(KEY);
if (result == null) {
result = new Object();
dcl.put(KEY, result);
}
return result;
}

What is dcl? A synchronised HashMap or Hashtable?

The first obvious problem is the getInternal is a race. Either
synchronise on the same key as dcl is using (if it using a single lock)
or use the java.util.concurrent.ConcurrentMap (or backport) and putIfAbsent.

Have you looked at the stack traces? They might make more sense if you
lock on an object having a giveaway class name.

So getInternal might look like:

public Object getInternal() { // internal and public??
synchronized (dcl) {
Object result = dcl.get(KEY);
if (result == null) {
// You may want to make Lock a static nested class.
class Lock { }
result = new Lock();
dcl.put(KEY, result);
}
return result;
}
}

Or with ConcurrentMap:

public Object getInternal() { // internal and public??
// Not necessary,
// but optimisation assuming KEY is usually present.
Object result = dcl.get(KEY);
if (result != null) {
return result;
}

// You may want to make Lock a static nested class.
class Lock { }
result = new Lock();
Object old = dcl.putIfAbsent(KEY, result);
return old==null ? result : old;
}

The second problem is potential deadlocks or at least unrelated locking
interference.

How does the rest of the program use those objects as locks? String in
particular could be locked by anyone.

Assuming dcl is synchronised in someway, you have nested synchronisation
(always something to be exceedingly careful about). You synchronise on
value (whatever that might be) and then on dcl.

If you aren't careful about what you synchronise on then you can see odd
effects. Something else might hold the lock for an extended duration.
Other code wait/notify mechanisms might interfere (infamously, Thread is
synchronised and notified when it exits).

Tom Hawtin