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

[David Holmes]

Hi Brent,

On 11/05/2016 7:56 AM, Brent Christian wrote:
> Hi!  Welcome back to 8029891, "Deadlock detected in
> java/lang/ClassLoader/deadlock/GetResource.java".  Our previous
> discussion is at [1].
>
> Briefly, java.util.Properties isa Hashtable, which synchronizes on
> itself for any access. System.getProperties() returns the Properties
> instance accessed by the system, which any application code might
> synchronize on (that's what the test is doing).  System properties are a
> common way for changing default setting, so it's impractical to expect
> the class loading code path not to call System.getProperty.
>
> The fix is to change Properties to store its values in an internal
> ConcurrentHashMap, ignoring its Hashtable heritage.  In this way,
> Properties can be (partly) "de-sychronized", and deadlocks avoided.

As before I don't have any major concerns with this approach.

> While good progress was made during the original code review, all of the
> overridden methods in Properties caused an explosion of unnecessary
> JavaDoc (see specdiff at [2]).  With the recent fix of 8073100 (new
> "@hidden" JavaDoc tag), we can now avoid the additional clutter.

The existing javadoc has a section "Methods inherited from 
java.util.Hashtable" which I can't see in your specdiff - what does that 
section say about the methods you overrode to delegate to the CHM 
instance? Are they simply not listed, or does it lie and claim they are 
inherited, or does it have some new way to present "@hidden" things?

> Highlights from the previous discussion:
>
> * Specifics of where to remove synchronization: generally, read methods
> will be de-synchronized, while write/bulk-read operations will stay
> synchronized.
>
> * We considered whether we might be able use special-case code just for
> system properties, instead of a general change to Properties.  Because a
> user is able to supply their own Properties object to
> System.setProperties(), which is set as the system properties, and
> current behavior reflects changes made to original Properties object, it
> would be best to change the Properties class itself.
>
> * Don't leak extra add/remove functionality through returned Enumerations.
>
> * I said I would check on the synchronization of serialization.
> writeHashtable() looks okay to me; I expect this is not such a concern
> for readHashtable().
>
>
> A new webrev is here:
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/
>
> Other changes since the last webrev I posted:
> * In readHashtable(), I added a check to validate 'elements', as is done
> in Hashtable.
>
> * I have indicated that Iterators from
> [keySet|entrySet|values].iterator() no longer fail-fast.  While perhaps
> not strictly necessary, given the "no guarantees" nature of the
> Hashtable spec, my personal preference is to make it clear that
> Properties no longer makes a best (or any) effort to throw
> ConcurrentModificationException.  This should not affect any working
> code (which would not see a CME anyway).  Given that Properties are
> meant to be used as a collection of largely static values, adding
> additional code to maintain fail-fast behavior is not warranted.
>
>
> FWIW, the deadlock code path is now different than what is seen in the
> bug report (see [3]).  We're now getting hung up on the
> Integer.getInteger() call to get MAX_ARITY on line 60 of
> MethodHandleImpl [4].

So while the original deadlock is resolved by all this change, there 
still exists a deadlock. I can see from the details that store() locks 
the properties object and can lead to MethodHandleNatives.linkCallsite, 
while in another thread we have

at java.util.Hashtable.get(java.base at 9-ea/Hashtable.java:370)
	- waiting to lock <0x00000006cff097f0> (a java.util.Properties)
...
at java.lang.invoke.CallSite.<clinit>(java.base at 9-ea/CallSite.java:230)
	at 
java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(java.base at 9-ea/MethodHandleNatives.java:250)

I think we have some fundamental initialization order problems that need 
to be addressed in a holistic way rather than deadlock by deadlock (or 
initialization failure by initialization failure).

Thanks,
David

>
> Thanks,
> -Brent
>
> 1.
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033147.html
> 2.
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.3/specdiff-plain/Properties.html
>
> 3. http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/NewDeadlock.txt
> 4.
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/1049321b86cb/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
>
>
>