Hi Jaikiran,
Code changes look OK to me , couple of minor comments
1-> you don't need "this" to access stop(this.stop)
2-> i think super.stop() should be the first line in the overridden stop
method.
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.
Thanks,
Vyom
On Sat, Aug 31, 2019 at 9:37 AM Jaikiran Pai <jai.forums2...@gmail.com>
wrote:
> Hello Daniel,
>
> On 30/08/19 9:14 PM, Daniel Fuchs wrote:
> > Hi Jaikiran,
> >
> > 1075 System.out.println("Server will stop
> > accepting connections due to an exception");
> > 1076 io.printStackTrace(System.out);
> > 1077 break;
> >
> > Two things here:
> >
> > 1.if ss.connect() throws, it may be because ss is closed, or not.
> > To be on the safe side, we should call ss.close() before break;
>
> You are right. I have now updated this part of the code.
>
>
> > 2.if the application calls stop() I expect that the call to ss.accept()
> > will throw.
> >
> > Therefore I think that the exception should be simply skipped
> > if stop == true (that's normal termination, we don't want to pollute
> > the log with stack traces of exceptions that are expected),
> > but we should probably always print it on System.err if stop == false
> > otherwise we will be reduced to guessing if we see the client side
> > failing with Connection Refused.
> >
> Agreed. This has now been updated too.
>
> The updated webrev is at
> http://cr.openjdk.java.net/~jpai/webrev/8223714/2/webrev/. I have also
> included you as the reviewer in the commit message.
>
>
> > Otherwise, I believe it looks good. Thanks for another good fix :-)
>
> Thank you :)
>
> -Jaikiran
>
>
>
--
Thanks,
Vyom
