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/
