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
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
