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

[David Holmes]

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