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

From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of Brent Christian <brent.christ...@oracle.com> Sent: Tuesday, May 17, 2016 8:46 PM To: Mandy Chung; David Holmes Cc: core-libs-dev Subject: Re: RFR 8029891 : Deadlock detected i

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

Hi Brent, The unsynchronized test looks good. Let's hope that we didn't miss any hidden detail. We'll see how this works out when people get hold of new build containing this change. Regards, Peter On 05/18/2016 03:46 AM, Brent Christian wrote: Hi! Here's the final(?) webrev:

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

On 18/05/2016 11:46 AM, Brent Christian wrote: Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: No further comments from me. Thanks, David * The @hidden JavaDoc tag has been removed. It can't be

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

> On May 17, 2016, at 6:46 PM, Brent Christian > wrote: > > Hi! > > Here's the final(?) webrev: > http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ +1 Nit: line 132, 134 in PropertiesSerialization.java test are long lines that should be wrapped to

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

Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: * The @hidden JavaDoc tag has been removed. It can't be used due to methods disappearing from the API page. Living with the additional JavaDoc is

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

> On May 12, 2016, at 5:58 PM, David Holmes wrote: >> >> With all of the inherited methods @hidden, it looks like that section >> is left out altogether. > > Okay, so I have to say @hidden seems to me to be seriously flawed! If a class > has a method that can be

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

> On May 12, 2016, at 4:55 PM, Brent Christian > wrote: > > Update to the test, and additional comments: > > http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/ > +1 Mandy

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

Good idea. Mandy > On May 13, 2016, at 12:10 AM, Peter Levart wrote: > > Hi Brent, > > > This looks good. It might also be a good idea to add a test that checks this > new implementation detail (the unsynchronized get) that new code will become > dependent upon, for

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

Note that there is a possible initialization circularity introduced by this patch, which I think is harmless because: - ThreadLocalRandom has just recently been enhanced to cope with such situations - CHM needs ThreadLocalRandom in put() for example, when new entry is being inserted, TLR

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

Hi Brent, This looks good. It might also be a good idea to add a test that checks this new implementation detail (the unsynchronized get) that new code will become dependent upon, for example: /* * @test * @bug 8029891 * @summary Test that the Properties.get() method does not

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

Hi Brent, On 13/05/2016 7:02 AM, Brent Christian wrote: Hi, David On 5/11/16 8:39 PM, David Holmes wrote: On 11/05/2016 7:56 AM, Brent Christian wrote: While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary

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

Update to the test, and additional comments: http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/ Thanks, -Brent On 5/12/16 4:15 PM, Mandy Chung wrote: On May 12, 2016, at 2:44 PM, Brent Christian wrote: On 5/12/16 11:44 AM, Mandy Chung wrote: This

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

> On May 12, 2016, at 2:44 PM, Brent Christian > wrote: > > On 5/12/16 11:44 AM, Mandy Chung wrote: >> >> This patch looks good. >> >> To help future readers to understand this, it may be better to move: >> 1152 private transient ConcurrentHashMap

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

On 5/12/16 11:44 AM, Mandy Chung wrote: This patch looks good. To help future readers to understand this, it may be better to move: 1152 private transient ConcurrentHashMap map = 1153 new ConcurrentHashMap<>(8); to the beginning and add a comment describing

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

Hi, David On 5/11/16 8:39 PM, David Holmes wrote: On 11/05/2016 7:56 AM, Brent Christian wrote: 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

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

> On May 11, 2016, at 8:39 PM, David Holmes wrote: > >> 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

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

Hi Brent, > > A new webrev is here: > http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/ This patch looks good. To help future readers to understand this, it may be better to move: 1152 private transient ConcurrentHashMap map = 1153 new

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

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.

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

On 5/10/16 2:56 PM, Brent Christian wrote: 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

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

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

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

Hi, Daniel You're right, thanks. The situation is the same for elements(). I've updated these in my local repo. I checked through the methods that return a Set/Enumeration/etc, and I believe there's also an issue with entrySet(). The returned Set should be backed by the map, and support

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

On May 15, 2015, at 12:09 PM, Brent Christian brent.christ...@oracle.com wrote: Updated webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/ I have restored synchronization to modification methods, and to my interpretation of bulk read operations: * forEach *

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

On 16/05/2015 5:09 AM, Brent Christian wrote: On 5/13/15 8:53 AM, Mandy Chung wrote: On May 12, 2015, at 2:26 PM, Peter Levart peter.lev...@gmail.com wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all

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

Hi Brent, 1163 @Override 1164 public EnumerationObject keys() { 1165 return map.keys(); 1166 } I wonder if you should use: public EnumerationObject keys() { return Collections.enumeration(map.keySet()); } instead. ConcurrentHashMap.keys() returns an Enumeration which

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

On 5/13/15 8:53 AM, Mandy Chung wrote: On May 12, 2015, at 2:26 PM, Peter Levart peter.lev...@gmail.com wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so

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

On 05/13/2015 01:45 AM, Paul Sandoz wrote: On May 12, 2015, at 10:49 PM, Mandy Chung mandy.ch...@oracle.com wrote: How likely does existing code do this? A proper way is to call System.setProperty. One pattern I found on System.setProperties is like this to add a system property of

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

On May 12, 2015, at 10:49 PM, Mandy Chung mandy.ch...@oracle.com wrote: Ah, I understand Mandy now. You are talking about using special Properties implementation just for system properties. Unfortunately, this is currently valid code: Properties props = new Properties(); ...

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

On May 12, 2015, at 2:26 PM, Peter Levart peter.lev...@gmail.com wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification

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

On 05/12/2015 07:41 AM, Peter Levart wrote: Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a

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

On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made

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

On 05/11/2015 11:41 PM, Peter Levart wrote: On 05/12/2015 07:41 AM, Peter Levart wrote: Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to

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

Hi Brent, On 05/04/2015 09: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/ Thanks for taking this on. I

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

Hi, Mandy. Thanks for having a look. On 5/11/15 12:09 PM, Mandy Chung wrote: ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input reader/stream/xml file is loaded and parsed rather than adding one entry at a time. I also like

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

On 05/12/2015 01:43 AM, Brent Christian wrote: Hi, Mandy. Thanks for having a look. On 5/11/15 12:09 PM, Mandy Chung wrote: ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input reader/stream/xml file is loaded and parsed

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

On May 11, 2015, at 4:43 PM, Brent Christian brent.christ...@oracle.com wrote: Hi, Mandy. Thanks for having a look. On 5/11/15 12:09 PM, Mandy Chung wrote: ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input

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

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

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

On 06/05/15 11:41, David Holmes wrote: 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

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

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

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

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

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

On 05/04/2015 06:11 PM, 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

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

Hi Brent, I think all you need for test is this: import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; public class CheckOverrides { public static void main(String[] args) {

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

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