Hello Vyom, Daniel,
On 02/09/19 3:00 PM, Daniel Fuchs wrote:
> 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?)
This does look better. I have updated the code to use this construct and
the new webrev is at
http://cr.openjdk.java.net/~jpai/webrev/8223714/3/webrev/ (only the
stop() method code has changed compared to previous webrev).
-Jaikiran
