RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly

[Daniel Fuchs]

Hi Vyom,

Now I have second thoughts. The documentation of poll(2) says:

        int poll(struct pollfd *fds, nfds_t nfds, int timeout);

        [...]

        Specifying a negative value in timeout means an infinite
        timeout.
        Specifying a timeout of zero causes poll() to return
        immediately, even if no file descriptors are ready.

As Michael hinted, are we sure that we are handling the
timeout=0 case properly?

Moreover, the timeout parameter is supposed to be an int.
I'd be more satisfied if we declared an int variable e.g.

    int timeoutms = timeout;

at about line 411, and then update it just after line 446:

    timeoutms = nanoTimeout / NET_NSEC_PER_MSEC

What do you think?

Also JDK-8179905 [1] also modified aix_close.c, solaris_close.c,
bsd_close.c [2] - so I believe you should also fix all these places.

best regards,

-- daniel

[1] https://bugs.openjdk.java.net/browse/JDK-8179905
[2] http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/cc195249c879

On 10/03/2020 16:53, Vyom Tewari26 wrote:
> 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.fuchs at oracle.com>
>     To: Vyom Tewari26 <vtewar26 at in.ibm.com>, net-dev at 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;
>      >           }
>      >
>      > ############################################################
>      >
> 
>