Hi Jaikiran,
Code changes look OK to me , couple of minor comments
1-> you don't need "this" to access stop(this.stop)
2-> i think super.stop() should be the first line in the overridden stop method.
3-> it is matter of personal preference but my preference is use the new variable name 'running' to check if server is running in place of 'stop' but again it is just a matter of personal preference.
Thanks,
Vyom
----- Original message -----
From: Jaikiran Pai <jai.forums2...@gmail.com>
Sent by: "net-dev" <net-dev-boun...@openjdk.java.net>
To: Daniel Fuchs <daniel.fu...@oracle.com>, "net-dev@openjdk.java.net" <net-dev@openjdk.java.net>
Cc:
Subject: [EXTERNAL] Re: RFR: 8223714: HTTPSetAuthenticatorTest could be made more resilient
Date: Sat, Aug 31, 2019 9:38 AM
Hello Daniel,
On 30/08/19 9:14 PM, Daniel Fuchs wrote:
> Hi Jaikiran,
>
> 1075 System.out.println("Server will stop
> accepting connections due to an exception");
> 1076 io.printStackTrace(System.out);
> 1077 break;
>
> Two things here:
>
> 1.if ss.connect() throws, it may be because ss is closed, or not.
> To be on the safe side, we should call ss.close() before break;
You are right. I have now updated this part of the code.
> 2.if the application calls stop() I expect that the call to ss.accept()
> will throw.
>
> Therefore I think that the exception should be simply skipped
> if stop == true (that's normal termination, we don't want to pollute
> the log with stack traces of exceptions that are expected),
> but we should probably always print it on System.err if stop == false
> otherwise we will be reduced to guessing if we see the client side
> failing with Connection Refused.
>
Agreed. This has now been updated too.
The updated webrev is at
http://cr.openjdk.java.net/~jpai/webrev/8223714/2/webrev/ . I have also
included you as the reviewer in the commit message.
> Otherwise, I believe it looks good. Thanks for another good fix :-)
Thank you :)
-Jaikiran
