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

[Gary Adams]

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.

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.

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