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

[Peter Levart]

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 synchronize 
any more
  * @run main CheckUnsynchronizedGet
  */
public class CheckUnsynchronizedGet {

     public static void main(String[] args) {
         Properties props = new Properties();
         synchronized (props) {
             props.setProperty("key", "value");
             String value =
                 CompletableFuture.supplyAsync(() -> 
props.getProperty("key")).join();
             assert "value".equals(value);
         }
     }
}

What do you think?

Regards, Peter

On 05/13/2016 01:55 AM, Brent Christian wrote:
> 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 
>>> <brent.christian at oracle.com> 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<Object, Object> map =
>>>> 1153             new ConcurrentHashMap<>(8);
>>>>
>>>> to the beginning and add a comment describing what are lock-free 
>>>> and what are synchronized (basically some part of your summary below).
>>>>
>>>> Also a comment in Hashtable::defaultWriteHashtable and 
>>>> readHashtable methods to mention that they are overridden by 
>>>> Properties.
>>>
>>> Good ideas.
>>>
>>>> In the GetResource.java test, what is the reason taking out:
>>>>    73   URL u2 = 
>>>> Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
>>>>
>>>
>>> It was coming back null and failing the test, I think because
>>> Jigsaw moved CalendarData into a different module (jdk.localedata, I 
>>> believe?).
>>
>> src/java.base/share/classes/sun/util/resources/CalendarData.properties 
>> is still in java.base.  This is due to resource encapsulation. 
>> Unnamed module calling ClassLoader::getResource of a resource in a 
>> named module returns null.
>>
>>
>>>   I fiddled with @modules a bit, but stopped when I discovered that 
>>> when the test deadlocks, it hangs on the first call to getResource() 
>>> and doesn't get to this line.
>>>
>>> I can look into getting it to load CalendarData again, if you like.
>>
>> The launcher and builtin class loader implementation has changed so 
>> much that the code path exercised JDK-6977738 no longer exists.  I 
>> think dropping line 73 is okay.
>>
>> Mandy
>>