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

[Daniel Fuchs]

Hi Brent,

1163     @Override
1164     public Enumeration<Object> keys() {
1165         return map.keys();
1166     }

I wonder if you should use:

public Enumeration<Object> 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
>