RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly

[Michael McMahon]

As it happens, I'm not sure that NET_Timeout is ever called with timeout 
= 0.

A zero value for the socket option means block forever and there is no 
support
for polling in the API.

- Michael.

On 10/03/2020 18:21, Daniel Fuchs wrote:
> 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;
>>      >           }
>>      >
>>      > ############################################################
>>      >
>>
>>
>