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