Hi Daniel, Thanks for review, please find my answers below.
1-> What is the purpose of calling NativeThread.close? To close the thread which is waiting in SocketAccept, I wrote a test in such a way that the server thread will wait infinitely in socket accept and there will not be any incoming socket so accept will never return. As Alan suggested we should have an incoming connection to test if accept works after SIGPIPE. I will change the test as per his suggestion and I think we don't need the "close" method. 2-> When does the test succeed? test succeeds if no SocketTimeoutException is thrown when you send a SIGPIPE to the server thread. If you run the same test without a fix, the test will throw a SocketTimeoutException. ##################################### Sending SIGPIPE to ServerSocket thread. Exception in thread "Thread-0" java.lang.RuntimeException: java.net.SocketTimeoutException: Accept timed out at SocketAcceptTest$Server.run(SocketAcceptTest.java:89) at java.base/java.lang.Thread.run(Thread.java:832) Caused by: java.net.SocketTimeoutException: Accept timed out at java.base/java.net.PlainSocketImpl.socketAccept(Native Method) ############################################ 3-> Looking at SocketAcceptInterruptTest it looks like the test will succeed if it manages to send the signal - and that's it. Is it really a measure of success? No, I explained in question 2. Use of CountdownLatch will be good, let me rewrite the test I will try to use if it is possible. Thanks, Vyom On Wed, Jun 24, 2020 at 4:11 PM Daniel Fuchs <daniel.fu...@oracle.com> wrote: > Hi Vyom, > > What is the purpose of calling NativeThread.close? > Is that just to clean up and make the server thread terminate? > If so there should be better ways to do that. > > When does the test succeed? > > Looking at SocketAcceptInterruptTest it looks like the test > will succeed if it manages to send the signal - and that's it. > Is it really a measure of success? > > Also it seems like better synchronization primitive could > be used (for instance CountDownLatch) than relying on arbitrary > timeout. In particular `isSet` could be a CountDownLatch. > > best regards, > > -- daniel > > On 24/06/2020 05:53, Vyom Tiwari wrote: > > Hi Daniel/Alan, > > > > Thanks for the review , I was not sure how JVM/JDK handles the signals > > that's why I installed my own signal handler. As you both pointed out, I > > removed the custom signal handler in NativeThread. > > > > Please find the latest > > webrev(http://cr.openjdk.java.net/~vtewari/8237858/webrev0.5/index.html) > > > where i have incorporated most of the review comments. > > > > Thanks, > > Vyom > -- Thanks, Vyom
