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

[serguei.spitsyn at oracle.com]

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 serguei.spitsyn at oracle.com 
>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
>> <mailto: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/
>> <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