Hi Alex, > Of course error can be cleared before each try - there is not functional > difference.
It is just a little confusing, as you can get an exception in L. 95, too. But I'm ok with it, if you prefer it like this. I would suggest, though, to sleep some ms before a retry and double the sleep time in each following retry. Best regards, Richard. -----Original Message----- From: Alex Menkov <[email protected]> Sent: Freitag, 6. September 2019 22:52 To: Reingruber, Richard <[email protected]>; [email protected]; OpenJDK Serviceability <[email protected]> Subject: Re: RFR: JDK-8192057: com/sun/jdi/BadHandshakeTest.java fails with java.net.ConnectException Hi Richard, On 09/06/2019 09:28, Reingruber, Richard wrote: > Hi Alex, > > that's a good fix for the issue. > > One minor thing: > > 89 Exception error = null; > 90 for (int retry = 0; retry < 5; retry++) { > 91 try { > 92 log("retry: " + retry); > 93 s = new Socket("localhost", port); > 94 error = null; > 95 > s.getOutputStream().write("JDWP-".getBytes("UTF-8")); > 96 break; > 97 } catch (ConnectException ex) { > 98 log("got exception: " + ex.toString()); > 99 error = ex; > 100 } > 101 } > 102 if (error != null) { > 103 throw error; > 104 } > > Is there a reason to clear the local variable error in line 94 instead of > clearing it > in line 91 where each new attempt begins? The logic here is: The cycle has 2 exits: - error (max retry attempts reached, error is set by last "catch") - success ("break" statement at line 96, error should be null) So error is cleared only after the socket is connected (this is the problematic operation which can cause ConnectException). Of course error can be cleared before each try - there is not functional difference. --alex > > Cheers, Richard. > > -----Original Message----- > From: serviceability-dev <[email protected]> On > Behalf Of [email protected] > Sent: Mittwoch, 4. September 2019 22:11 > To: Alex Menkov <[email protected]>; OpenJDK Serviceability > <[email protected]> > Subject: Re: RFR: JDK-8192057: com/sun/jdi/BadHandshakeTest.java fails with > java.net.ConnectException > > Hi Alex, > > The fix looks good. > Good simplification! > > Thanks, > Serguei > > > On 9/4/19 12:19, Alex Menkov wrote: >> Hi all, >> >> Please review the fix for BadHandshakeTest test. >> The problem is the test connects to the server twice and if debuggee >> hasn't yet handled disconnection, the next connect gets "connection >> refused" error. >> Instead of adding delay before 2nd connect (we never know "good" value >> for the delay and big delay can cause "accept timeout"), the test >> re-tries connect in case of ConnectException. >> Also improved/simplified the test slightly - debuggee is now run with >> auto port assignment (used lib.jdb.Debuggee test class which >> implements required functionality). >> >> jira: >> https://bugs.openjdk.java.net/browse/JDK-8192057 >> webrev: >> http://cr.openjdk.java.net/~amenkov/jdk14/BadHandshakeTest/webrev/ >> >> --alex >
