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.bate...@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
