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.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;
> }
>
> ############################################################
>
