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