RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java

[Brent Christian]

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