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

[Reingruber, Richard]

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
>