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

[serguei.spitsyn at oracle.com]

Hi Alex,

It looks good.

Thanks,
Serguei

On 9/10/19 1:33 PM, Alex Menkov wrote:
> 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
>>>