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

[Brent Christian]

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.

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.

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


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