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

Wed, 10 Oct 2018 19:41:16 -0700 

On 10/10/18 19:36, David Holmes 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>    -> 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. 

It is better to have res initialized:
  int res = -1;

Thanks,
Serguei


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 [email protected] <mailto:[email protected]> <[email protected] <mailto:[email protected]>> 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

Reply via email to