RFR: JDK-8218754: JDK-8068225 regression in JDIBreakpointTest

[serguei.spitsyn at oracle.com]

Hi Gary,

+1

Thanks,
Serguei

On 2/12/19 11:24 AM, Chris Plummer wrote:
> Hi Gary,
>
> The changes look good.
>
> thanks,
>
> Chris
>
> On 2/12/19 11:04 AM, gary.adams at oracle.com wrote:
>> Successfully ran 100X {solaris,macosx,windows,linux} debug builds
>> for the tests impacted by Debugee.endDebugee() processing.
>> |test/hotspot/jtreg/vmTestbase/nsk/jdi 
>> test/hotspot/jtreg/vmTestbase/vm/mlvm test/jdk/com/sun/jdi|
>> On 2/12/19 7:40 AM, Gary Adams wrote:
>>> On 2/12/19, 7:14 AM, David Holmes wrote:
>>>> On 12/02/2019 10:11 pm, Gary Adams wrote:
>>>>> Yes, see the revised webrev, we do have to guard against
>>>>> multiple calls to dispose, e.g. catch and ignore 
>>>>> VMDisconnectException.
>>>>> We don't need to guard against a null vm. That would only exist if
>>>>> the vm was never initialized and we were shutting down.
>>>>
>>>> Okay. Revised webrev is better in that regard.
>>> One more round to add the import for VMDisconnectedException.
>>>
>>>   Webrev: http://cr.openjdk.java.net/~gadams/8218754/webrev.01/
>>>>
>>>>> There is a race condition between the debuggee and the debugger 
>>>>> process.
>>>>> In the original endDebugee, it attempted to do 2 things in parallel.
>>>>> By calling dispose then waitFor, in most cases the debugee would 
>>>>> finish
>>>>> and report status before all the dispose operations completed.
>>>>> But some tests would fail if the dispose happened quicker than the 
>>>>> debuggee
>>>>> could report final exit status.
>>>>>
>>>>> The tests based on JDIBreakpointTest on  the other hand don't really
>>>>> care about the the final exit status. Once all the events have 
>>>>> been seen and handled, the test wants to shutdown the session.
>>>>
>>>> I'm still not seeing how JDIBreakpointTest started 
>>>> hanging/timing-out after you switched the order of dispose and 
>>>> waitFor. Does dispose affect the debuggee process or the debugger 
>>>> process?
>>> The JDIBreakpointTest would timeout on the endDebugee call to waitFor.
>>> There was no reason for the debugee to exit at that point.
>>> The call to dispose, provided the debugee with it's reason to exit.
>>> Not sure which operation induces the debugee to exit.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> On 2/12/19, 6:59 AM, David Holmes wrote:
>>>>>> Hi Gary,
>>>>>>
>>>>>> On 12/02/2019 8:08 pm, gary.adams at oracle.com wrote:
>>>>>>> The recent change to JDK-8068225 changed the order of operations
>>>>>>> in Debugee.endDebugee() to wait for the debugee to exit before
>>>>>>> disposing of the vm on the debugger side of the connection.
>>>>>>> For the tests based on JDIBreakpointTest the debuggee exit
>>>>>>> status is not used and the tests relied on the
>>>>>>> debugger side dispose operation to end the test.
>>>>>>>
>>>>>>> Since JDIBreakpointTest already includes a call to wait for
>>>>>>> the debugee, if does not need to use endDebuggee()
>>>>>>> to dispose and wait for the debugee to finish.
>>>>>>
>>>>>> I agree that potentially calling waitFor twice seems pointless. 
>>>>>> But how did the reordering of vm.dispose() and waitFor() cause 
>>>>>> all these tests to hang if they were waiting anyway? Does 
>>>>>> vm.dispose() have an effect on destroying the process?
>>>>>>
>>>>>> Also what concerns me is that dispose() is not resilient the way 
>>>>>> that endDebuggee is:
>>>>>>
>>>>>>  public void dispose() {
>>>>>>     vm.dispose();
>>>>>>  }
>>>>>>
>>>>>> versus
>>>>>>
>>>>>>     public int endDebugee() {
>>>>>>         if (vm != null) {
>>>>>>             try {
>>>>>>                 vm.dispose();
>>>>>>             } catch (VMDisconnectedException ignore) {
>>>>>>             }
>>>>>>             vm = null;
>>>>>>         }
>>>>>>
>>>>>> Do we need to be concerned with a null VM or getting 
>>>>>> VMDisconnectedException?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Testing in progress. The vm/mlvm tests are included in tiers 2, 
>>>>>>> 3 and 6.
>>>>>>>
>>>>>>> diff --git 
>>>>>>> a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
>>>>>>> b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
>>>>>>>
>>>>>>> --- 
>>>>>>> a/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
>>>>>>>
>>>>>>> +++ 
>>>>>>> b/test/hotspot/jtreg/vmTestbase/vm/mlvm/share/jdi/JDIBreakpointTest.java 
>>>>>>>
>>>>>>> @@ -1,5 +1,5 @@
>>>>>>>   /*
>>>>>>> - * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All 
>>>>>>> rights reserved.
>>>>>>> + * Copyright (c) 2011, 2019, Oracle and/or its affiliates. All 
>>>>>>> rights reserved.
>>>>>>>    * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>>>>    *
>>>>>>>    * This code is free software; you can redistribute it and/or 
>>>>>>> modify it
>>>>>>> @@ -359,7 +359,7 @@
>>>>>>>           }.go();
>>>>>>>
>>>>>>>           if (!debuggee.terminated())
>>>>>>> -            debuggee.endDebugee();
>>>>>>> +            debuggee.dispose();
>>>>>>>
>>>>>>>           debuggee.waitFor();
>>>>>>>           return true;
>>>>>>>
>>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190212/7977d291/attachment-0001.html>