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

JC Beyler jcbeyler at google.com
Thu Oct 11 03:28:45 UTC 2018


Whomp whomp :)

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

On Wed, Oct 10, 2018 at 8:12 PM David Holmes <david.holmes at oracle.com>
wrote:

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


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181010/f69109dd/attachment-0001.html>


More information about the serviceability-dev mailing list