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

[Jie Fu]

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