RFR JDK-8195703: BasicJDWPConnectionTest.java: 'App exited unexpectedly with 2'

[Alex Menkov]

Hi all,

please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
webrev:
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/

I was not able to reproduce the issue, but accordingly the logs in jira
root cause is a transport initialization error "Address already in use".
The test uses Utils.getFreePort() to select some free port, but it can
be race condition when some other app (or other test) uses the port
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
Other changes:
- dropped catching exceptions and calling System.exit() - this causes
SecurityException in JTReg harness which makes error analysis much harder;
- dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java test;
jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and restarting
debuggee, so I keep it as is.

--alex

[s***@oracle.com]

Hi Alex,

It looks good to me.
How did you test it?

Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
I was not able to reproduce the issue, but accordingly the logs in
jira root cause is a transport initialization error "Address already
in use".
The test uses Utils.getFreePort() to select some free port, but it can
be race condition when some other app (or other test) uses the port
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
- dropped catching exceptions and calling System.exit() - this causes
SecurityException in JTReg harness which makes error analysis much harder;
- dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java test;
  jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and
restarting debuggee, so I keep it as is.
--alex

[JC Beyler]

Hi Alex,

-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
-> Why not make it javadoc like the other methods of the same file
(so @return instead of returns and a second * at the start of the comment)?

-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html
a) I'm surprised by this:

+ int res;+ try {+ res =
handshake(detectPort(a.getProcessStdout()));+ } finally {
a.stopApp();+ }
if (res < 0) {


I would have thought that this makes javac return a "res might not be
initialized" error.


b) Nit: Is there a reason we are complicating the code here:

try {
LingeredApp a = LingeredApp.startApp(cmd);++
// startApp is expected to fail, but if not, terminate the app+
try {+ a.stopApp();+ } catch
(IOException e) {+ // print and let the test fail+
System.err.println("LingeredApp.stopApp failed");+
e.printStackTrace();+ }
} catch (IOException ex) {
System.err.println(testName + ": caught expected IOException");
System.err.println(testName + " PASSED");
return;
}

Why not just put it below? We could either put a outside the try and
then move that code out; or perhaps move it into a separate method to
let

the reader concentrate on the test at hand and let the "stopping of
the app" happen somewhere else?


Thanks,
Jc

Post by s***@oracle.com
Hi Alex,
It looks good to me.
How did you test it?
Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
I was not able to reproduce the issue, but accordingly the logs in
jira root cause is a transport initialization error "Address already
in use".
The test uses Utils.getFreePort() to select some free port, but it can
be race condition when some other app (or other test) uses the port
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
- dropped catching exceptions and calling System.exit() - this causes
SecurityException in JTReg harness which makes error analysis much harder;
- dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java test;
jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and
restarting debuggee, so I keep it as is.
--alex

[David Holmes]

Post by s***@oracle.com
Hi Alex,
-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>
  -> Why not make it javadoc like the other methods of the same file
-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html>
+ int res;
+ try {
+ res = handshake(detectPort(a.getProcessStdout()));
+ } finally {
a.stopApp();
+ }
if (res < 0) {
I would have thought that this makes javac return a "res might not be initialized" error.

res can only be uninitialized if an exception occurs, in which case you
won't reach the "if (res , 0)" statement.

David
-----

Post by s***@oracle.com
try {
LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
} catch (IOException ex) {
System.err.println(testName + ": caught expected IOException");
System.err.println(testName + " PASSED");
return;
}
Why not just put it below? We could either put a outside the try and then move that code out; or perhaps move it into a separate method to let
the reader concentrate on the test at hand and let the "stopping of the app" happen somewhere else?
Thanks,
Jc
Hi Alex,
It looks good to me.
How did you test it?
Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/

<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>

Post by Alex Menkov
I was not able to reproduce the issue, but accordingly the logs in
jira root cause is a transport initialization error "Address already
in use".
The test uses Utils.getFreePort() to select some free port, but

it can

Post by Alex Menkov
be race condition when some other app (or other test) uses the port
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
- dropped catching exceptions and calling System.exit() - this

causes

Post by Alex Menkov
SecurityException in JTReg harness which makes error analysis much harder;
- dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java test;
  jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and
restarting debuggee, so I keep it as is.
--alex

--
Thanks,
Jc

[s***@oracle.com]

Post by David Holmes

Post by s***@oracle.com
Hi Alex,
-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>
   -> Why not make it javadoc like the other methods of the same file
-
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html>
+ int res;
+ try {
+ res = handshake(detectPort(a.getProcessStdout()));
+ } finally {
          a.stopApp();
+ }
          if (res < 0) {
I would have thought that this makes javac return a "res might not be initialized" error.

res can only be uninitialized if an exception occurs, in which case
you won't reach the "if (res , 0)" statement.

It is better to have res initialized:
  int res = -1;

Thanks,
Serguei

Post by David Holmes
David
-----

Post by s***@oracle.com
          try {
              LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
          } catch (IOException ex) {
              System.err.println(testName + ": caught expected
IOException");
              System.err.println(testName + " PASSED");
              return;
          }
Why not just put it below? We could either put a outside the try and
then move that code out; or perhaps move it into a separate method to
let
the reader concentrate on the test at hand and let the "stopping of
the app"  happen somewhere else?
Thanks,
Jc
    Hi Alex,
    It looks good to me.
    How did you test it?
    Thanks,
    Serguei
     > Hi all,
     >
     > please review a fix for
     > https://bugs.openjdk.java.net/browse/JDK-8195703
     > http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>
     >
     > I was not able to reproduce the issue, but accordingly the
logs in
     > jira root cause is a transport initialization error "Address
already
     > in use".
     > The test uses Utils.getFreePort() to select some free port, but
    it can
     > be race condition when some other app (or other test) uses the
port
     > selected before debuggee application starts to listen on it.
     > The fix uses dynamic port allocation and then parses it from the
     > debuggee output.
     > - dropped catching exceptions and calling System.exit() - this
    causes
     > SecurityException in JTReg harness which makes error analysis
much
     > harder;
     > - dropped using of Utils.getFreePort() from
jdi/DoubleAgentTest.java
     > test;
     >   jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
     > handles "Already in use" error re-peeking other free port and
     > restarting debuggee, so I keep it as is.
     >
     > --alex
     >
--
Thanks,
Jc

[JC Beyler]

So that's what I thought too but I did this:

$ cat Test.java
public class Test {
public static void main(String[] args) {
int res;

try {
res = 5;
} catch (Exception e) {
}

if (res > 0) {
}
}
}

That fails to compile with JDK8 and JDK11 it seems. Perhaps I'm doing
something wrong?

$ javac Test.java
Test.java:10: error: variable res might not have been initialized
if (res > 0) {
^
1 error


Sorry if I'm derailing the review :)

Jc

Post by JC Beyler

Post by s***@oracle.com
Hi Alex,
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by s***@oracle.com
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by s***@oracle.com
-> Why not make it javadoc like the other methods of the same file

comment)?

Post by s***@oracle.com
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html

Post by s***@oracle.com
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html

Post by s***@oracle.com
+ int res;
+ try {
+ res = handshake(detectPort(a.getProcessStdout()));
+ } finally {
a.stopApp();
+ }
if (res < 0) {
I would have thought that this makes javac return a "res might not be

initialized" error.
res can only be uninitialized if an exception occurs, in which case you
won't reach the "if (res , 0)" statement.
David
-----

Post by s***@oracle.com
try {
LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
} catch (IOException ex) {
System.err.println(testName + ": caught expected

IOException");

Post by s***@oracle.com
System.err.println(testName + " PASSED");
return;
}
Why not just put it below? We could either put a outside the try and

then move that code out; or perhaps move it into a separate method to let

Post by s***@oracle.com
the reader concentrate on the test at hand and let the "stopping of the

app" happen somewhere else?

Post by s***@oracle.com
Thanks,
Jc
Hi Alex,
It looks good to me.
How did you test it?
Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/

<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>

Post by Alex Menkov
I was not able to reproduce the issue, but accordingly the logs in
jira root cause is a transport initialization error "Address

already

Post by s***@oracle.com

Post by Alex Menkov
in use".
The test uses Utils.getFreePort() to select some free port, but

it can

Post by Alex Menkov
be race condition when some other app (or other test) uses the

port

Post by s***@oracle.com

Post by Alex Menkov
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
- dropped catching exceptions and calling System.exit() - this

causes

Post by Alex Menkov
SecurityException in JTReg harness which makes error analysis much
harder;
- dropped using of Utils.getFreePort() from

jdi/DoubleAgentTest.java

Post by s***@oracle.com

Post by Alex Menkov
test;
jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and
restarting debuggee, so I keep it as is.
--alex

--
Thanks,
Jc

[David Holmes]

Post by JC Beyler
$ cat Test.java
public class Test {
public static void main(String[] args) {
int res;
try {
res = 5;
} catch (Exception e) {
}
if (res > 0) {
}
}
}
That fails to compile with JDK8 and JDK11 it seems. Perhaps I'm doing something wrong?

Yes. :) You are catching the theoretical exception from the assignment
and so allowing control to reach the if statement - at which point res
may not be initialized.

David
-----

Post by JC Beyler
$ javac Test.java
Test.java:10: error: variable res might not have been initialized
if (res > 0) {
^
1 error
Sorry if I'm derailing the review :)
Jc

Post by s***@oracle.com
Hi Alex,
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>

Post by s***@oracle.com
    -> Why not make it javadoc like the other methods of the same

file
comment)?

Post by s***@oracle.com
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html>
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html>

Post by s***@oracle.com
+ int res;
+ try {
+ res = handshake(detectPort(a.getProcessStdout()));
+ } finally {
           a.stopApp();
+ }
           if (res < 0) {
I would have thought that this makes javac return a "res might

not be initialized" error.
res can only be uninitialized if an exception occurs, in which case you
won't reach the "if (res , 0)" statement.
David
-----

Post by s***@oracle.com
           try {
               LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
           } catch (IOException ex) {
               System.err.println(testName + ": caught expected

IOException");

Post by s***@oracle.com
               System.err.println(testName + " PASSED");
               return;
           }
Why not just put it below? We could either put a outside the try

and then move that code out; or perhaps move it into a separate
method to let

Post by s***@oracle.com
the reader concentrate on the test at hand and let the "stopping

of the app"  happen somewhere else?

Post by s***@oracle.com
Thanks,
Jc
     Hi Alex,
     It looks good to me.
     How did you test it?
     Thanks,
     Serguei
      > Hi all,
      >
      > please review a fix for
      > https://bugs.openjdk.java.net/browse/JDK-8195703
      >

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>

Post by s***@oracle.com
     <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>
      >
      > I was not able to reproduce the issue, but accordingly the

logs in

Post by s***@oracle.com
      > jira root cause is a transport initialization error

"Address already

Post by s***@oracle.com
      > in use".
      > The test uses Utils.getFreePort() to select some free

port, but

Post by s***@oracle.com
     it can
      > be race condition when some other app (or other test) uses

the port

Post by s***@oracle.com
      > selected before debuggee application starts to listen on it.
      > The fix uses dynamic port allocation and then parses it

from the

Post by s***@oracle.com
      > debuggee output.
      > - dropped catching exceptions and calling System.exit() - this
     causes
      > SecurityException in JTReg harness which makes error

analysis much

Post by s***@oracle.com
      > harder;
      > - dropped using of Utils.getFreePort() from

jdi/DoubleAgentTest.java

Post by s***@oracle.com
      > test;
      >   jdi/BadHandshakeTest.java also uses Utils.getFreePort(),

but it

Post by s***@oracle.com
      > handles "Already in use" error re-peeking other free port and
      > restarting debuggee, so I keep it as is.
      >
      > --alex
      >
--
Thanks,
Jc

--
Thanks,
Jc

[JC Beyler]

Whomp whomp :)

Ok, then my review becomes: apart from my nit, looks good to me :-)
Jc

Post by JC Beyler

Post by JC Beyler
$ cat Test.java
public class Test {
public static void main(String[] args) {
int res;
try {
res = 5;
} catch (Exception e) {
}
if (res > 0) {
}
}
}
That fails to compile with JDK8 and JDK11 it seems. Perhaps I'm doing

something wrong?
Yes. :) You are catching the theoretical exception from the assignment
and so allowing control to reach the if statement - at which point res
may not be initialized.
David
-----

Post by JC Beyler
$ javac Test.java
Test.java:10: error: variable res might not have been initialized
if (res > 0) {
^
1 error
Sorry if I'm derailing the review :)
Jc

Post by s***@oracle.com
Hi Alex,
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by JC Beyler
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by JC Beyler
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by JC Beyler

Post by s***@oracle.com
-> Why not make it javadoc like the other methods of the same

file
comment)?

Post by s***@oracle.com
-

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html

Post by JC Beyler
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html

Post by JC Beyler
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html

Post by JC Beyler

Post by s***@oracle.com
+ int res;
+ try {
+ res = handshake(detectPort(a.getProcessStdout()));
+ } finally {
a.stopApp();
+ }
if (res < 0) {
I would have thought that this makes javac return a "res might

not be initialized" error.
res can only be uninitialized if an exception occurs, in which case

you

Post by JC Beyler
won't reach the "if (res , 0)" statement.
David
-----

Post by s***@oracle.com
try {
LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
} catch (IOException ex) {
System.err.println(testName + ": caught expected

IOException");

Post by s***@oracle.com
System.err.println(testName + " PASSED");
return;
}
Why not just put it below? We could either put a outside the try

and then move that code out; or perhaps move it into a separate
method to let

Post by s***@oracle.com
the reader concentrate on the test at hand and let the "stopping

of the app" happen somewhere else?

Post by s***@oracle.com
Thanks,
Jc
Hi Alex,
It looks good to me.
How did you test it?
Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703

http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>

Post by s***@oracle.com
<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/>

Post by JC Beyler

Post by s***@oracle.com

Post by Alex Menkov
I was not able to reproduce the issue, but accordingly the

logs in

Post by s***@oracle.com

Post by Alex Menkov
jira root cause is a transport initialization error

"Address already

Post by s***@oracle.com

Post by Alex Menkov
in use".
The test uses Utils.getFreePort() to select some free

port, but

Post by s***@oracle.com
it can

Post by Alex Menkov
be race condition when some other app (or other test) uses

the port

Post by s***@oracle.com

Post by Alex Menkov
selected before debuggee application starts to listen on

it.

Post by JC Beyler

Post by s***@oracle.com

Post by Alex Menkov
The fix uses dynamic port allocation and then parses it

from the

Post by s***@oracle.com

Post by Alex Menkov
debuggee output.
- dropped catching exceptions and calling System.exit() -

this

Post by JC Beyler

Post by s***@oracle.com
causes

Post by Alex Menkov
SecurityException in JTReg harness which makes error

analysis much

Post by s***@oracle.com

Post by Alex Menkov
harder;
- dropped using of Utils.getFreePort() from

jdi/DoubleAgentTest.java

Post by s***@oracle.com

Post by Alex Menkov
test;
jdi/BadHandshakeTest.java also uses Utils.getFreePort(),

but it

Post by s***@oracle.com

Post by Alex Menkov
handles "Already in use" error re-peeking other free port

and

Post by JC Beyler

Post by s***@oracle.com

Post by Alex Menkov
restarting debuggee, so I keep it as is.
--alex

--
Thanks,
Jc

--
Thanks,
Jc

[Alex Menkov]

Hi Jc,

updated webrev:
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.02/

Post by s***@oracle.com
<http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>

Post by JC Beyler
-> Why not make it javadoc like the other methods of the same file

     comment)?

done.

Post by s***@oracle.com

Post by JC Beyler
           try {
               LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
           } catch (IOException ex) {
               System.err.println(testName + ": caught expected

     IOException");

Post by JC Beyler
               System.err.println(testName + " PASSED");
               return;
           }
Why not just put it below? We could either put a outside the try

and then move that code out; or perhaps move it into a separate
method to let

Post by JC Beyler
the reader concentrate on the test at hand and let the "stopping

     of the app"  happen somewhere else?

This is to improve code cleanup on error (the test expects that
LingeredApp.startApp(cmd) fails, but if the test fails, the app remains
running).
I moved cleanup outside of the try (and removed try/catch around
a.stopApp() - the test throws Exception anyway)

--alex

[JC Beyler]

Hi Alex,

Looks great to me, thanks for cleaning it up!
Jc

Post by Alex Menkov
Hi Jc,
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.02/

<

http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

Post by JC Beyler

Post by JC Beyler
-> Why not make it javadoc like the other methods of the same file

comment)?

done.

Post by JC Beyler

Post by JC Beyler
try {
LingeredApp a = LingeredApp.startApp(cmd);
+
+ // startApp is expected to fail, but if not, terminate the app
+ try {
+ a.stopApp();
+ } catch (IOException e) {
+ // print and let the test fail
+ System.err.println("LingeredApp.stopApp failed");
+ e.printStackTrace();
+ }
} catch (IOException ex) {
System.err.println(testName + ": caught expected

IOException");

Post by JC Beyler
System.err.println(testName + " PASSED");
return;
}
Why not just put it below? We could either put a outside the try

and then move that code out; or perhaps move it into a separate
method to let

Post by JC Beyler
the reader concentrate on the test at hand and let the "stopping

of the app" happen somewhere else?

This is to improve code cleanup on error (the test expects that
LingeredApp.startApp(cmd) fails, but if the test fails, the app remains
running).
I moved cleanup outside of the try (and removed try/catch around
a.stopApp() - the test throws Exception anyway)
--alex

[Alex Menkov]

Hi Serguei,

I run the test itself and jdk/com/sun/jdi multiple times.

--alex

Post by s***@oracle.com
Hi Alex,
It looks good to me.
How did you test it?
Thanks,
Serguei

Post by Alex Menkov
Hi all,
please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8195703
http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/
I was not able to reproduce the issue, but accordingly the logs in
jira root cause is a transport initialization error "Address already
in use".
The test uses Utils.getFreePort() to select some free port, but it can
be race condition when some other app (or other test) uses the port
selected before debuggee application starts to listen on it.
The fix uses dynamic port allocation and then parses it from the
debuggee output.
- dropped catching exceptions and calling System.exit() - this causes
SecurityException in JTReg harness which makes error analysis much harder;
- dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java test;
  jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it
handles "Already in use" error re-peeking other free port and
restarting debuggee, so I keep it as is.
--alex