<div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Alex,<br><div><br></div><div>- <a href="http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html">http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html</a><br></div><div>  -> 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)?</div><div><br></div><div>- <a href="http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html">http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html</a><br></div><div>a)   I'm surprised by this:</div><div><pre style="color:rgb(0,0,0)"><span class="gmail-new" style="color:blue">+        int res;</span>
<span class="gmail-new" style="color:blue">+        try {</span>
<span class="gmail-new" style="color:blue">+            res = handshake(detectPort(a.getProcessStdout()));</span>
<span class="gmail-new" style="color:blue">+        } finally {</span>
         a.stopApp();
<span class="gmail-new" style="color:blue">+        }</span>
         if (res < 0) {</pre><pre style="color:rgb(0,0,0)"><br></pre><pre style="color:rgb(0,0,0)">I would have thought that this makes javac return a "res might not be initialized" error. </pre><pre style="color:rgb(0,0,0)"><br></pre><pre style="color:rgb(0,0,0)">b) Nit: Is there a reason we are complicating the code here:</pre><pre style="color:rgb(0,0,0)"><pre>         try {
             LingeredApp a = LingeredApp.startApp(cmd);
<span class="gmail-new" style="color:blue">+</span>
<span class="gmail-new" style="color:blue">+            // startApp is expected to fail, but if not, terminate the app</span>
<span class="gmail-new" style="color:blue">+            try {</span>
<span class="gmail-new" style="color:blue">+                a.stopApp();</span>
<span class="gmail-new" style="color:blue">+            } catch (IOException e) {</span>
<span class="gmail-new" style="color:blue">+                // print and let the test fail</span>
<span class="gmail-new" style="color:blue">+                System.err.println("LingeredApp.stopApp failed");</span>
<span class="gmail-new" style="color:blue">+                e.printStackTrace();</span>
<span class="gmail-new" style="color:blue">+            }</span>
         } catch (IOException ex) {
             System.err.println(testName + ": caught expected IOException");
             System.err.println(testName + " PASSED");
             return;
         }</pre><pre>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</pre><pre>the reader concentrate on the test at hand and let the "stopping of the app"  happen somewhere else?</pre><pre><br></pre></pre></div><div>Thanks,</div><div>Jc</div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 10, 2018 at 5:18 PM <a href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <<a href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Alex,<br>
<br>
It looks good to me.<br>
How did you test it?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/10/18 16:25, Alex Menkov wrote:<br>
> Hi all,<br>
><br>
> please review a fix for<br>
> <a href="https://bugs.openjdk.java.net/browse/JDK-8195703" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8195703</a><br>
> webrev:<br>
> <a href="http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/</a><br>
><br>
> I was not able to reproduce the issue, but accordingly the logs in <br>
> jira root cause is a transport initialization error "Address already <br>
> in use".<br>
> The test uses Utils.getFreePort() to select some free port, but it can <br>
> be race condition when some other app (or other test) uses the port <br>
> selected before debuggee application starts to listen on it.<br>
> The fix uses dynamic port allocation and then parses it from the <br>
> debuggee output.<br>
> Other changes:<br>
> - dropped catching exceptions and calling System.exit() - this causes <br>
> SecurityException in JTReg harness which makes error analysis much <br>
> harder;<br>
> - dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java <br>
> test;<br>
>   jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it <br>
> handles "Already in use" error re-peeking other free port and <br>
> restarting debuggee, so I keep it as is.<br>
><br>
> --alex<br>
><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>