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

[David Holmes]

On 6/05/2015 5:57 PM, Peter Levart wrote:
>
>
> 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.

What I meant was to continue to delegate to the CHM to replace the 
inherited Hashtable methods, but to keep the synchronized on the 
Properties specific methods.

Cheers,
David
-----

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