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

[David Holmes]

On 11/10/2018 1:09 PM, JC Beyler wrote:
> 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?

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

> $ 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
> 
> 
> On Wed, Oct 10, 2018 at 7:36 PM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     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>
> 
>      >
>     <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>
> 
>      >
>     <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>
>      > <mailto:serguei.spitsyn at oracle.com
>     <mailto:serguei.spitsyn at oracle.com>> <serguei.spitsyn at oracle.com
>     <mailto:serguei.spitsyn at oracle.com>
>      > <mailto: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/>
>      >     <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
> 
> 
> 
> -- 
> 
> Thanks,
> Jc