Hi Daniel,
In PlainSocketImpl.socketAccept() we are taking care of 0 timeout as follows.
/* passing a timeout of 0 to poll will return immediately,
but in the case of ServerSocket 0 means infinite. */
if (timeout <= 0) {
ret = NET_Timeout(env, fd, -1, 0);
} else {
ret = NET_Timeout(env, fd, nanoTimeout / NET_NSEC_PER_MSEC, prevNanoTime);
}
but in the case of ServerSocket 0 means infinite. */
if (timeout <= 0) {
ret = NET_Timeout(env, fd, -1, 0);
} else {
ret = NET_Timeout(env, fd, nanoTimeout / NET_NSEC_PER_MSEC, prevNanoTime);
}
You are right that timeout parameter should be int but if you see the code we storing current time in nanosecond and manipulating the timeout . If we use int then there may be integer overflow.
I think changes are good & safe and we don't need to do any additional changes.
Thanks,
Vyom
----- Original message -----
From: Daniel Fuchs <daniel.fu...@oracle.com>
To: Vyom Tewari26 <vtewa...@in.ibm.com>
Cc: net-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
Date: Tue, Mar 10, 2020 10:51 PM
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.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;
> > }
> >
> > ############################################################
> >
>
>
