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

[Jie Fu]

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.

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
>>>>
>>>>
>>>
>>