RE: RFR: 8223714: HTTPSetAuthenticatorTest could be made more resilient

[Vyom Tewari26]

Hi Jaikiran,

 

Please find my comments inline.

 

----- Original message -----
From: Jaikiran Pai <jai.forums2...@gmail.com>
To: Vyom Tewari26 <vtewa...@in.ibm.com>
Cc: daniel.fu...@oracle.com, net-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 8223714: HTTPSetAuthenticatorTest could be made more resilient
Date: Mon, Sep 2, 2019 9:21 AM
 

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.

My only worry was what if super.stop() fails and throws exception but in super.stop() we are catching/ignoring the exception and logging the trace to system.out.

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

that is fine as you said it is private variable and changing name will not make much difference.

 

Thanks,

Vyom

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

-Jaikiran