RFR: JDK-8216528: test/jdk/java/rmi/transport/runtimeThreadInheritanceLeak/RuntimeThreadInheritanceLeak.java failing with Xcomp
Roger Riggs
Roger.Riggs at oracle.com
Thu Jan 31 15:16:14 UTC 2019
Pushed
On 01/30/2019 10:12 PM, Jie Fu wrote:
> Hi Roger,
>
> I really hope you can still sponsor this.
> Could you help me, please?
> Thanks again.
>
> Best regards,
> Jie
>
>
> On 2019/1/31 上午10:59, David Holmes wrote:
>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>
More information about the core-libs-dev
mailing list