Hi Daniel,
Thanks for review and running all tests, In past we(i am the culprit) removed the "if(timeout > 0) {" condition by mistake. This check is to make sure that if the timeout set only then we do all other time calculation but somehow it was not very clear by reading the code.
NET_Timeout does not tells that there is infinite(-1) timeout as well, if you see the code changes I explicitly put else block to make code more readable.
I hosted the patch to cr.openjdk.net(http://cr.openjdk.java.net/~vtewari/8237858/webrev/index.html).
Thanks,
Vyom
----- Original message -----
From: Daniel Fuchs <daniel.fu...@oracle.com>
To: Vyom Tewari26 <vtewa...@in.ibm.com>, net-dev@openjdk.java.net
Cc:
Subject: [EXTERNAL] Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
Date: Tue, Mar 10, 2020 9:07 PM
Hi Vyom,
I have sent your proposed fix to our test system and observed no
regression. I agree your proposed changes seem to address the
issue adequately. However, I'd like to hear a second opinion on the
possible side effects of this fix, since NET_Timeout may be called
at many other places.
I see that Alan has commented on
https://bugs.openjdk.java.net/browse/JDK-8237858
It would be good to get his approval (or at least Michael McMahon's)
before pushing.
best regards,
-- daniel
On 25/02/2020 16:36, Vyom Tewari26 wrote:
> Hi ,
> Please find the below fix for the
> issue(https://bugs.openjdk.java.net/browse/JDK-8237858 ). In
> "PlainSocketImpl_socketAccept" did not handle -1 timeout properly.
> In case of -1 timeout, "PlainSocketImpl_socketAccept" calls
> "NET_Timeout" if it is interrupted by signal(EINTR) then in case of -1
> timeout it returns immediately instead of looping again.
> Thanks,
> Vyom
> ##########################################################
> diff -r d6b968af8b65 src/java.base/linux/native/libnet/linux_close.c
> --- a/src/java.base/linux/native/libnet/linux_close.c Mon Feb 24
> 23:44:29 2020 -0500
> +++ b/src/java.base/linux/native/libnet/linux_close.c Tue Feb 25
> 19:06:11 2020 +0530
> @@ -437,12 +437,16 @@
> * has expired return 0 (indicating timeout expired).
> */
> if (rv < 0 && errno == EINTR) {
> - jlong newNanoTime = JVM_NanoTime(env, 0);
> - nanoTimeout -= newNanoTime - prevNanoTime;
> - if (nanoTimeout < NET_NSEC_PER_MSEC) {
> - return 0;
> + if(timeout > 0) {
> + jlong newNanoTime = JVM_NanoTime(env, 0);
> + nanoTimeout -= newNanoTime - prevNanoTime;
> + if (nanoTimeout < NET_NSEC_PER_MSEC) {
> + return 0;
> + }
> + prevNanoTime = newNanoTime;
> + } else {
> + continue; // timeout is -1, so loop again.
> }
> - prevNanoTime = newNanoTime;
> } else {
> return rv;
> }
>
> ############################################################
>
