RFR: JDK-8192057: com/sun/jdi/BadHandshakeTest.java fails with java.net.ConnectException

[Alex Menkov]

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.menkov at oracle.com>
> Sent: Freitag, 6. September 2019 22:52
> To: Reingruber, Richard <richard.reingruber at sap.com>; serguei.spitsyn at oracle.com; OpenJDK Serviceability <serviceability-dev at 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-bounces at openjdk.java.net> On Behalf Of serguei.spitsyn at oracle.com
>> Sent: Mittwoch, 4. September 2019 22:11
>> To: Alex Menkov <alexey.menkov at oracle.com>; OpenJDK Serviceability <serviceability-dev at 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
>>