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.
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
