RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly

[Vyom Tiwari]

Hi Alan,

Thanks for the review, I  passed ss to sendSignal because in  failing cases
ss will throw an exception  on the first SIGPIPE signal so  i need to put a
check if the server socket is not closed.

Same reason for L64,in failing case server socket will be closed after
SIGPIPE. I will  address all your review comments.

Thanks,
Vyom

On Sat, Jul 4, 2020 at 11:41 AM Alan Bateman <Alan.Bateman at oracle.com>
wrote:

> On 30/06/2020 06:39, Vyom Tiwari wrote:
> >
> >
> > Thanks for review please find the latest
> > webrev(http://cr.openjdk.java.net/~vtewari/8237858/webrev0.8/index.html).
>
> > I fixed the Windows build issue.
> >
> Minor nit but I assume build-dev will want to keep the indentation in
> TestFilesCompilation.gmk consistent. Also the patch still has the
> formatting issues in linux_close.c and bsd_close.c. You thought it might
> be webrev but looks to me that the issues are in the patch file too.
>
> The updated tests are looking better. A few comments on
> SocketAcceptInterruptTest, most apply to SocketReadInterruptTest too:
> - "service" isn't a great name for the Executor. Also you can make use
> of try-finally, e.g.
> ExecutorService executor = Executors.newFixedThreadPool(1);
> try { ... } finally { executor.shutdown(); }
> - you can also use try-with-resources for the ServerSocket and it will
> avoid checking if ss is nul
> - sendSignal - not clear why this needs the "ss" parameter, maybe to
> make the testing of the buggy implementation more robust?
> - I assume thet isClosed check at L64 can be removed too.
> - is Server.isSet needed - the getID method just needs to spin until
> threadId != 0.
>
> -Alan.
>


-- 
Thanks,
Vyom
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20200704/7d12dc75/attachment.htm>