RFR: 8223714: HTTPSetAuthenticatorTest could be made more resilient

[Jaikiran Pai]

Hello Vyom,

Thank you for the review. Comments inline.

On 31/08/19 7:05 PM, Vyom Tewari26 wrote:
> Hi Jaikiran,
>  
> Code changes look OK to me , couple of minor comments
> 1-> you don't need "this" to access stop(this.stop)

Usage of this.member, is more of a personal practice that I try to
follow whenever possible, to avoid accidental shadowing of local
variables. Seeing the rest of the code (even one of my own reference to
stop where I didn't use this.stop), I think it makes it consistent not
to use this.stop. So I have gone ahead and updated the webrev (at the
same location[1]) to change this.stop to stop.


> 2-> i think super.stop() should be the first line in the overridden
> stop method.

I had thought of this when I introduced the change. However, I felt that
it's better to set the stop=true before calling the super.stop(), since
this way the other thread gets "notified" a bit early that it doesn't
have to accept any more connections, thus it doesn't have to first go
into a blocking accept() and then get thrown an exception (when the
stop() method does a ss.close()) and then go and check if it was asked
to stop. Of course, this doesn't guarantee that the other thread will
always benefit from this (since it might already be in a blocking
accept()), but at least it does present the other thread a chance.

If you and/or others still feel calling super.stop() should be the first
line, do let me know and I'll change accordingly.

> 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.
>  

If you don't have a strong preference, then I would like to stick with
"stop" for now, if you don't mind. Especially, given that this is a
private variable which never gets exposed outside (not even via some
getter/setter). 

Would you like me to add you as a reviewer for this patch?

-Jaikiran

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20190902/df2d2ec0/attachment.html>