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

[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

On Wed, Oct 10, 2018 at 5:18 PM serguei.spitsyn at oracle.com <
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/
> >
> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181010/c2146bd5/attachment.html>