<div dir="ltr"><div dir="ltr">Hi Jaikiran,</div>

<div dir="ltr"> </div>

<div dir="ltr">Code changes look OK to me , couple of minor comments</div>

<div dir="ltr">1-> you don't need "this" to access stop(this.stop)</div>

<div dir="ltr">2-> i think super.stop() should be the first line in the overridden stop method.</div>

<div dir="ltr">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.</div>

<div dir="ltr"> </div>

<div dir="ltr">Thanks,</div>

<div dir="ltr">Vyom</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 31, 2019 at 9:37 AM Jaikiran Pai <<a href="mailto:jai.forums2013@gmail.com">jai.forums2013@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Daniel,<br>
<br>
On 30/08/19 9:14 PM, Daniel Fuchs wrote:<br>
> Hi Jaikiran,<br>
><br>
> 1075                     System.out.println("Server will stop<br>
> accepting connections due to an exception");<br>
> 1076                     io.printStackTrace(System.out);<br>
> 1077                     break;<br>
><br>
> Two things here:<br>
><br>
> Â 1.if ss.connect() throws, it may be because ss is closed, or not.<br>
> Â Â  To be on the safe side, we should call ss.close() before break;<br>
<br>
You are right. I have now updated this part of the code.<br>
<br>
<br>
> Â 2.if the application calls stop() I expect that the call to ss.accept()<br>
> Â Â  will throw.<br>
><br>
> Â Â  Therefore I think that the exception should be simply skipped<br>
> Â Â  if stop == true (that's normal termination, we don't want to pollute<br>
> Â Â  the log with stack traces of exceptions that are expected),<br>
> Â Â  but we should probably always print it on System.err if stop == false<br>
> Â Â  otherwise we will be reduced to guessing if we see the client side<br>
> Â Â  failing with Connection Refused.<br>
><br>
Agreed. This has now been updated too.<br>
<br>
The updated webrev is at<br>
<a href="http://cr.openjdk.java.net/~jpai/webrev/8223714/2/webrev/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jpai/webrev/8223714/2/webrev/</a>. I have also<br>
included you as the reviewer in the commit message.<br>
<br>
<br>
> Otherwise, I believe it looks good. Thanks for another good fix :-)<br>
<br>
Thank you :)<br>
<br>
-Jaikiran<br>
<br>
<br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Thanks,<br>Vyom</div>