RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java (original) (raw)

Brent Christian brent.christian at oracle.com
Mon May 18 22:44:56 UTC 2015

Hi, Daniel

You're right, thanks.

The situation is the same for elements(). I've updated these in my local repo.

I checked through the methods that return a Set/Enumeration/etc, and I believe there's also an issue with entrySet(). The returned Set should be backed by the map, and support remove operations, but not add/addAll. However the Set returned from ConcurrentHashMap.entrySet() does provide add/addAll.

Sadly, there is no "unaddableSet()" in java.util.Collections. AFAICT, I'll need to add a new inner wrapper class for this. :\

Thanks, -Brent

On 5/15/15 12:58 PM, Daniel Fuchs wrote:

Hi Brent,

1163 @Override 1164 public Enumeration keys() { 1165 return map.keys(); 1166 } I wonder if you should use: public Enumeration keys() { return Collections.enumeration(map.keySet()); } instead. ConcurrentHashMap.keys() returns an Enumeration which is also an Iterator which supports removal of elements, that might have unintended side effects WRT to concurrency & subclassing. best regards, -- daniel On 15/05/15 21:09, Brent Christian wrote: On 5/13/15 8:53 AM, Mandy Chung wrote:

On May 12, 2015, at 2:26 PM, Peter Levart <peter.levart at gmail.com> wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote:

But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized.

This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. I agree that making read-only methods not synchronized while keeping any method with write-access with synchronized is pretty safe change and low compatibility risk. On the other hand, would most of the overrridden methods be synchronized then? Yes, and that doesn't seem to be a problem. Setting properties is not very frequent operation and is usually quite localized. Reading properties is, on the other hand, a very frequent operation dispersed throughout the code (almost like logging) and therefore very prone to deadlocks like the one in this issue. I think that by having an unsynchronized get(Property), most problems with Properties will be solved - deadlock and performance (scalability) related. I’m keen on cleaning up the system properties but it is a bigger scope as it should take other related enhancement requests into account that should be something for future. I think what you propose seems a good solution to fix JDK-8029891 while it doesn’t completely get us off the deadlock due to user code locking the Properties object. Updated webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/ I have restored synchronization to modification methods, and to my interpretation of "bulk read" operations: * forEach * replaceAll * store: (synchronized(this) in store0; also, entrySet() returns a synchronizedSet) * storeToXML: PropertiesDefaultsHandler.store() synchronizes on the Properties instance * propertyNames(): returns an Enumeration not backed by the Properies; calls (still synchronized) enumerate() * stringPropertyNames returns a Set not backed by the Properties; calls (still synchronized) enumerateStringProperties These continue to return a synchronized[Set|Collection]: * keySet * entrySet * values These methods should operate on a coherent snapshot: * clone * equals * hashCode I expect these two are only used for debugging. I wonder if we could get away with removing synchronization: * list * toString I'm still looking through the serialization code, though I suspect some synchronization should be added. The new webrev also has the updated test case from Peter. Thanks, -Brent

More information about the core-libs-dev mailing list