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

[Brent Christian]

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<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
>>
>