RFR: 8223714: HTTPSetAuthenticatorTest could be made more resilient

[Daniel Fuchs]

Hi,

On 02/09/2019 05:29, Vyom Tewari26 wrote:
>>     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.

I'd also prefer setting `stop` to  true before calling super.stop() for
the reason that Jaikiran states. That said maybe a better implementation
could be:

     try (var toClose = ss;) {
         stop = true;
         super.stop();
     } catch (IOException ex) {
         ...
     }

this way we can be sure that ss.close will be closed
even if super.stop() throws something (which shouldn't
happen but who knows?)

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

For volatile variables it's usually a better practice to use
some variable which doesn't need to be initialized to a non
default value. `running` would have to be initialized to `true`
somewhere. Let's stick with `stop` :-)

best regards,

-- daniel


> Forgot the link to the webrev. It's here http://cr.openjdk.java.net/~jpai/webrev/8223714/2/webrev/