Re: RFR[15] JDK-8238740: java/net/httpclient/whitebox/FlowTestDriver.java would not specify a TLS protocol
Hi John,
Looks good to me.
best regards,
-- daniel
On 02/03/2020 08:08, sha.ji...@oracle.com wrote:
Hi,
java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/FlowTest.java
would not use a specific TLS protocol, instead just use the default TLS
protocol, exactly TLSv1.3 now.
diff -r 4a5a7dc9d05c
test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/FlowTest.java
---
a/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/FlowTest.java
Sun Mar 01 17:36:03 2020 -0800
+++
b/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/FlowTest.java
Mon Mar 02 16:01:15 2020 +0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -80,7 +80,6 @@
SSLEngine engineClient = ctx.createSSLEngine();
SSLParameters params = ctx.getSupportedSSLParameters();
params.setApplicationProtocols(new String[]{"proto1",
"proto2"}); // server will choose proto2
- params.setProtocols(new String[]{"TLSv1.2"}); // TODO: This is
essential. Needs to be protocol impl
engineClient.setSSLParameters(params);
engineClient.setUseClientMode(true);
completion = new CompletableFuture<>();
Best regards,
John Jiang
Re: RFR: 8235459: HttpRequest.BodyPublishers#ofFile(Path) assumes the default file system
Julia, > On 5 Mar 2020, at 13:50, Julia Boes wrote: > > Hi, > > Please see this fix that adds support for non-default file systems to the > HttpClient. More specifically, the change is in > RequestPublishers.FilePublisher where an UnsupportedOperationException is > thrown if a java.io.File cannot be obtained. The exception is now caught and > a function is used to obtain an InputStream with a privileged scope based on > the captured AccessControlContext. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8235459 > > CSR: https://bugs.openjdk.java.net/browse/JDK-8240526 I added myself as reviewer. > Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8235459/webrev.01/ The change looks good, and the tests appear comprehensive. -Chris.
Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
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;
}
Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
The change looks okay to me. Though the comment about the -1 case also
applies
if timeout is 0. The behavior is still okay in that case, but the
comment should acknowledge that,
however unlikely it is to occur.
- Michael.
On 10/03/2020 16:37, Daniel Fuchs wrote:
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;
}
RE: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
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 To: Vyom Tewari26 , net-dev@openjdk.java.netCc:Subject: [EXTERNAL] Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectlyDate: Tue, Mar 10, 2020 9:07 PM
Hi Vyom,I have sent your proposed fix to our test system and observed noregression. I agree your proposed changes seem to address theissue adequately. However, I'd like to hear a second opinion on thepossible side effects of this fix, since NET_Timeout may be calledat many other places.I see that Alan has commented onhttps://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,-- danielOn 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;> }>> >
Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
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
To: Vyom Tewari26 , 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;
> }
>
>
>
Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
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
To: Vyom Tewari26 , 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;
> }
>
>
>
Re: RFR[8239355]: '(dc) Initial value of SO_SNDBUF should allow sending large datagrams (macOS)'
Hi Alan and Daniel, Thanks for the feedback. I've changed the test to be DatagramChannel specific, and it now checks getOption(SO_SNDBUF) for both IPv4 and IPv6. You can find the updated webrev below. http://cr.openjdk.java.net/~pconcannon/8239355/webrevs/webrev.02 Kind regards, Patrick On 09/03/2020 20:03, Alan Bateman wrote: On 09/03/2020 18:08, Daniel Fuchs wrote: Hi Alan, On 09/03/2020 17:01, Alan Bateman wrote: On 09/03/2020 12:39, Patrick Concannon wrote: bug: https://bugs.openjdk.java.net/browse/JDK-8239355 webrev: http://cr.openjdk.java.net/~pconcannon/8239355/webrevs/webrev.00/index.html This is a change to the DatagramChannel implementation. I guess I assumed there would be a DatagramChannel test that checked that getOption(SO_SNDBUF) returned a minimum value for both the IPv4 and IPv6 cases. +1. It's so obvious I didn't think of it when Patrick & I discussed the test. Okay, I think that would be useful to add as a macOS specific test. Also we can test DatagramChannel sending datagrams of maximum size on all platforms (doesn't need to be macOS specific). We probably don't need a DatagramSocket test for this change as it doesn't touch that code (and the code in PDSI isn't quite right). -Alan
Re: RFR[8239355]: '(dc) Initial value of SO_SNDBUF should allow sending large datagrams (macOS)'
On 10/03/2020 18:32, Patrick Concannon wrote: Hi Alan and Daniel, Thanks for the feedback. I've changed the test to be DatagramChannel specific, and it now checks getOption(SO_SNDBUF) for both IPv4 and IPv6. You can find the updated webrev below. http://cr.openjdk.java.net/~pconcannon/8239355/webrevs/webrev.02 Thanks for adding a test for getOption(SO_SNDBUF). That test (testGetOption) should be checking that SO_SNDBUF is >= expected value as it's okay for net.inet.udp.maxdgram to have a larger than what the test expects. testSend sends to the loopback address but I think we need this test to send datagrams on the network (sending to the loopback is okay too but I think you want this test to send a datagram on the network because we want fragmentation on the network(. The java.net.preferIPv6Addresses system property is about configuring the order of name service lookup. These runs shouldn't impact anything here, dual and preferIPv4Stack=true should be all that is needed. A minor nit is that we should probably find a name for the test that is consistent with the other tests in this area. Something like LargeDatagram or MinSendBufferSize is okay. -Alan
RE: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
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); }
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 To: Vyom Tewari26 Cc: net-dev@openjdk.java.netSubject: [EXTERNAL] Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectlyDate: 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 thetimeout=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_MSECWhat 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 > To: Vyom Tewari26 , 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.> >
RE: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
Hi Daniel/Michael,
Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8237858/webrev0.1/index.html) where i did the similar changes in (aix_close,solaris_close,bsd_close).c
files as well.
Thanks,
Vyom
- Original message -From: Michael McMahon Sent by: "net-dev" To: Daniel Fuchs Cc: net-dev@openjdk.java.netSubject: [EXTERNAL] Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectlyDate: Tue, Mar 10, 2020 10:56 PM
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 nosupportfor 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 >> To: Vyom Tewari26 , 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;>> > } el
