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
