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

Wed, 30 Jan 2019 19:01:26 -0800 

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

Reply via email to