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

[David Holmes]

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.

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

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