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

[Peter Levart]

On 05/05/2015 03:25 AM, David Holmes wrote:
> Hi Brent,
>
> On 5/05/2015 2:11 AM, Brent Christian wrote:
>> Hi,
>>
>> Please review this fix, courtesy of Peter Levart (thanks!), that I would
>> like to get in.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8029891
>> http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
>>
>> There is some discussion of it in the bug report, starting at 
>> 2014-12-31.
>>
>> The problem, as stated by Mandy:
>>
>> "System Properties is a hashtable that synchronizes on itself for any
>> access. Currently System.getProperties returns the Properties instance
>> accessed by the system in which any application code might synchronize
>> on it (that's what the test is doing). The problem reported JDK-6977738
>> is about Properties.store method that was fixed not to synchronize on
>> this instance. System property is a common way for changing the default
>> setting and so it's impractical to expect the class loading code path
>> not to call System.getProperty."
>>
>> This fix changes java.util.Properties to store its values in an internal
>> ConcurrentHashMap, ignoring its Hashtable heritage.  In this way,
>> Properties can be "de-sychronized": all methods inherited from Hashtable
>> are overridden, to remove synchronization, and delegate to the internal
>> CHM.
>
> I don't think you want to de-synchronize the load* methods - you don't 
> want two threads calling load concurrently. But that then raises the 
> problem of concurrent modification while a load is in progress. 
> Synchronization ensures serialization and by removing it you have done 
> more than just avoid deadlocks.
>
> I think this needs a more careful examination of the expected/desired 
> concurrent interactions between different methods. It may be that 
> simply not utilizing the synchronized Hashtable methods is sufficient 
> to resolve the deadlock, while still providing reasonable 
> serialization via the existing synchronized Properties methods - or it 
> may not. But allowing concurrent modifications will change behaviour 
> in an unexpected, and incompatible way, in my opinion.
>
> David
> -----

Hi David,

You say: "It may be that simply not utilizing the synchronized Hashtable 
methods is sufficient to resolve the deadlock, while still providing 
reasonable serialization via the existing synchronized Properties 
methods - or it may not".

How do you propose to not utilize synchronized Hashtable methods? By not 
utilizing Properties at all? This may be difficult to achieve as most 
system configuration is specified as system Properties.

So what about taking a more conservative approach by making all 
"modification" and "bulk" methods synchronized and exposing just 
single-entry "read-only" methods as not synchronized (mainly 
Hashatable.get())?

Regards, Peter

>
>> The serialized form is unchanged.
>>
>>
>> An alternative approach considered would be for System.getProperties()
>> to return a duplicate snapshot of the current Properties.  This presents
>> a compatibility risk to existing code that keeps a reference to the
>> return value of System.getProperties() and expects to either read new
>> properties added afterwards, or set properties on the cached copy.
>>
>> -Brent
>>