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

2019-02-12 Thread David Holmes 

On 12/02/2019 10:40 pm, 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/


Okay.




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.


I think there is a bigger problem underlying this. If the dispose() is 
needed to trigger the exit() then putting a waitFor before that will 
hang. But if the dispose() comes first then it can trigger the bug in 
8068225. So endDebuggee seems to be only usable in contexts where the 
debuggee is already ending/exiting now. That really doesn't make sense 
to me - why would you need to call endDebuggee if its already ending?


This fix addresses the current set of failures but I think more 
examination of this is needed.


Thanks,
David



Thanks,
David



On 2/12/19, 6:59 AM, David Holmes wrote:

Hi Gary,

On 12/02/2019 8:08 pm, gary.ad...@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;

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

2019-02-12 Thread serguei . spitsyn 

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.ad...@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.ad...@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;

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

2019-02-12 Thread Chris Plummer 

  
  
Hi Gary,
  
  The changes look good.
  
  thanks,
  
  Chris
  
  On 2/12/19 11:04 AM, gary.ad...@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.ad...@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

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

2019-02-12 Thread gary.ad...@oracle.com 

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.ad...@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;

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

2019-02-12 Thread Gary Adams 

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.ad...@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;

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

2019-02-12 Thread 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.ad...@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;

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

2019-02-12 Thread 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.ad...@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;

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

2019-02-12 Thread David Holmes 

Hi Gary,

On 12/02/2019 8:08 pm, gary.ad...@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;

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

2019-02-12 Thread Gary Adams 

Have to guard against multiple calls to dispose.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8218754/webrev.00/
  Issue: https://bugs.openjdk.java.net/browse/JDK-8218754


On 2/12/19, 5:08 AM, gary.ad...@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.

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;

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

2019-02-12 Thread gary.ad...@oracle.com 

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.

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;