RFR JDK-8195703: BasicJDWPConnectionTest.java: 'App exited unexpectedly with 2'
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Oct 11 02:39:56 UTC 2018
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
More information about the serviceability-dev
mailing list