Hi Alex, thanks, the change looks good.
Best regards, Richard. -----Original Message----- From: Alex Menkov <alexey.men...@oracle.com> Sent: Dienstag, 10. September 2019 22:33 To: Reingruber, Richard <richard.reingru...@sap.com>; serguei.spit...@oracle.com; OpenJDK Serviceability <serviceability-dev@openjdk.java.net> Subject: Re: RFR: JDK-8192057: com/sun/jdi/BadHandshakeTest.java fails with java.net.ConnectException Hi Richard, Updated webrev: http://cr.openjdk.java.net/~amenkov/jdk14/BadHandshakeTest/webrev.2/ added delay between retries & moved error clearing to the beginning of the cycle. --alex On 09/09/2019 02:07, Reingruber, Richard wrote: > 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 <alexey.men...@oracle.com> > Sent: Freitag, 6. September 2019 22:52 > To: Reingruber, Richard <richard.reingru...@sap.com>; > serguei.spit...@oracle.com; OpenJDK Serviceability > <serviceability-dev@openjdk.java.net> > 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 <serviceability-dev-boun...@openjdk.java.net> On >> Behalf Of serguei.spit...@oracle.com >> Sent: Mittwoch, 4. September 2019 22:11 >> To: Alex Menkov <alexey.men...@oracle.com>; OpenJDK Serviceability >> <serviceability-dev@openjdk.java.net> >> 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 >>
