RFR: JDK-8216528: test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failing with Xcomp

[David Holmes]

On 31/01/2019 12:44 pm, Jie Fu wrote:
> Hi David,
> 
> I prefer the original patch[1].
> 
> Could you please sponsor this issue or help me find a sponsor.
> I really appreciate it. Thank you very much.

Hopefully Roger will still sponsor this.

Thanks,
David

> Also thanks Roger. We had a pleasant discussion offlist.
> 
> Best regards,
> Jie
> 
> [1] 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.html 
> 
> 
> 
> On 2019/1/31 上午10:09, David Holmes wrote:
>> Hi Jie, Roger,
>>
>> I think this has now consumed far too many cycles for everyone, 
>> dealing with a test that is checking for a leak that can't even exist 
>> any more. Alan was fine with the original proposed patch, as was I, so 
>> I think we can should proceed with that. Obviously there is more than 
>> one way to tackle the Xcomp problem here, and there will always be 
>> issues with any test relying on GC interaction, but the proposed patch 
>> seems "good enough" to me.
>>
>> Cheers,
>> David
>>
>> On 29/01/2019 11:46 am, Jie Fu wrote:
>>> Hi,
>>>
>>> I agree that the simplest way to fix the issue is just adding the 
>>> reachabilityFence.
>>> But this patch might also fail since the VM doesn't guarantee that a 
>>> GC would be performed.
>>>
>>> I didn't make such patch since I've learned from Sergey and Alan that 
>>> calling "System.gc()" several times is unreliable to trigger a gc[1].
>>> So I still prefer the original one[2].
>>>
>>> Thanks a lot.
>>>
>>> Best regards,
>>> Jie
>>>
>>> [1] 
>>> https://mail.openjdk.java.net/pipermail/beans-dev/2019-January/000396.html 
>>>
>>> [2] 
>>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057852.html 
>>>
>>>
>>>
>>> On 2019/1/28 下午11:39, Roger Riggs wrote:
>>>> Hi,
>>>>
>>>> The simplest fix for this failing test is to add a call to 
>>>> reachabilityFence to prevent
>>>> the loader and loaderRef from going out of scope early.  It 
>>>> maintains getting debug
>>>> output on a failure.
>>>>
>>>> Offlist, Jie and I explored some alternate ways to write the test 
>>>> and settled on this one.
>>>>
>>>> Please review:
>>>>
>>>> diff --git 
>>>> a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>>>> b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>>>>
>>>> --- 
>>>> a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>>>>
>>>> +++ 
>>>> b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>>>>
>>>> @@ -113,11 +113,13 @@ public class RuntimeThreadInheritanceLea
>>>>              Reference dequeued = refQueue.remove(TIMEOUT);
>>>>              if (dequeued == null) {
>>>>                  System.err.println(
>>>> -                    "TEST FAILED: loader not deteced weakly 
>>>> reachable");
>>>> +                    "TEST FAILED: loader not detected weakly 
>>>> reachable");
>>>>                  dumpThreads();
>>>>                  throw new RuntimeException(
>>>>                      "TEST FAILED: loader not detected weakly 
>>>> reachable");
>>>>              }
>>>> +            Reference.reachabilityFence(loader);
>>>> +            Reference.reachabilityFence(loaderRef);
>>>>
>>>>              System.err.println(
>>>>                  "TEST PASSED: loader detected weakly reachable");
>>>>
>>>> Thanks, Roger
>>>>
>>>>
>>>>
>>>> On 01/11/2019 07:25 PM, Jie Fu wrote:
>>>>> Thanks David and Roger.
>>>>>
>>>>>
>>>>> On 2019年01月12日 06:52, David Holmes wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 12/01/2019 2:22 am, Roger Riggs wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> The proposed patch changes the test in a way that is unintended.
>>>>>>>
>>>>>>> Adding the infinite loop of gc() and sleep, will change the 
>>>>>>> timeout behavior
>>>>>>> from the existing timeout of TIMEOUT to the jtreg default timeout 
>>>>>>> of the whole test.
>>>>>>
>>>>>> Partially true. If the new loop gets stuck then yes the jtreg 
>>>>>> default timeout will apply - I don't see that is necessarily a bad 
>>>>>> thing. The existing timeout only applies to the refQueue.remove 
>>>>>> operation itself, you don't know how much time was spent before 
>>>>>> you got there, nor how much will be spent after in the 
>>>>>> dumpThreads() calls - so the jtreg timeout can still come into 
>>>>>> affect.
>>>>>>
>>>>>>> Further, it renders the check at lines 114-120 irrelevant since 
>>>>>>> loaderRef.get()
>>>>>>> will have returned null and the ref will have been enqueued by then.
>>>>>>
>>>>>> I wouldn't say irrelevant as it double-checks the interaction 
>>>>>> between the ref.get() and the queue.remove() - the result of one 
>>>>>> should imply the result of the other, but if enqueuing had a bug ....
>>>>>>
>>>>>>> While it is true that calling gc() only once is unreliable, a 
>>>>>>> better fix is to
>>>>>>> put the code from 108-120 in a loop with a fixed number of durations
>>>>>>
>>>>>> That would also work - say 5 loops and reduce TIMEOUT to 4000.
>>>>>>
>>>>>>> and add Reachability.reachabilityFence(loaderRef) to ensure the 
>>>>>>> ref is not ignored.
>>>>>>
>>>>>> Adding ReachabilityFence, alone, may solve the observed problem 
>>>>>> given one gc() seems to be working in practice (and because we 
>>>>>> don't actually have the leaked loaders anymore because those 
>>>>>> threads (sun.misc.GC threads) don't exist anymore).
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>>> Regards, Roger
>>>>>
>>>>>
>>>>
>>>
>