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

[Mandy Chung]

Good idea.

Mandy

> On May 13, 2016, at 12:10 AM, Peter Levart <peter.levart at gmail.com> 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 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
>>> 
>