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

[David Holmes]

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
> 
> 
> On 01/11/2019 12:07 AM, Jie Fu wrote:
>> Hi David,
>>
>> Thank you very much. I'd like to choose option 2.
>> A test case is more valuable if it can be used for both interpreter 
>> and JIT tests.
>>
>> Here is the patch based on your comments.
>> ---------------------------------------------------------------------------------- 
>>
>> diff -r 02e648ae46c3 
>> test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>>
>> --- 
>> a/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>> Wed Jan 09 01:06:19 2019 +0100
>> +++ 
>> b/test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java 
>> Fri Jan 11 12:55:38 2019 +0800
>> @@ -22,7 +22,7 @@
>>   */
>>
>>  /* @test
>> - * @bug 4404702
>> + * @bug 4404702 8216528
>>   * @summary When the RMI runtime (lazily) spawns system threads that 
>> could
>>   * outlive the application context in which they were (happened to be)
>>   * created, such threads should not inherit (thread local) data 
>> specific to
>> @@ -106,7 +106,10 @@
>>               * context class loader-- by giving it a chance to pass 
>> away.
>>               */
>>              Thread.sleep(2000);
>> -            System.gc();
>> +            while (loaderRef.get() != null) {
>> +                System.gc();
>> +                Thread.sleep(100);
>> +            }
>>
>>              System.err.println(
>>                  "waiting to be notified of loader being weakly 
>> reachable...");
>> ---------------------------------------------------------------------------------- 
>>
>>
>> Could you please review it and give me some advice?
>> Thanks.
>>
>> Best regards,
>> Jie
>>
>>
>> On 2019/1/11 下午12:16, David Holmes wrote:
>>>
>>> I see three choices for you here :)
>>>
>>> 1. Don't try to run all tests under Xcomp but just stick to the 
>>> "core" sets of tests already tested by others.
>>>
>>> 2. Fix the given test as outlined. (I tested it on linux-x64 and it 
>>> fixed the problem.)
>>>
>>> 3. Exclude the given test from Xcomp by adding: @requires vm.compMode 
>>> != "Xcomp"
>>>
>>> If you chose options 2 or 3 please update the @bug line with 8216528.
>>>
>>> The core-libs folk may have more to say here and they will need to 
>>> provide a sponsor for the commit.
>>>
>>> Thanks,
>>> David
>>> -----
>>
>