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

[David Holmes]

On 11/10/2018 12:19 PM, JC Beyler wrote:
> 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 
> (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 
> <http://cr.openjdk.java.net/%7Eamenkov/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.

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

David
-----

> 
> 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
> 
> On Wed, Oct 10, 2018 at 5:18 PM serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com>> wrote:
> 
>     Hi Alex,
> 
>     It looks good to me.
>     How did you test it?
> 
>     Thanks,
>     Serguei
> 
> 
>     On 10/10/18 16:25, Alex Menkov wrote:
>      > 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/
>     <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.
>      > 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
>      >
> 
> 
> 
> -- 
> 
> Thanks,
> Jc