Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Ivan Gerasimov]

Hi Chris!

A couple of minor comments.

1)
if (s != -1 || (s == -1 && errno != EINVAL)) {

This can be simplified to

if (s != -1 || errno != EINVAL) {

2)
What about sockets created with accept():  Shouldn't NET_Accept be 
modified to set O_CLOEXEC as well?

On Linux accept4() can be used to pass SOCK_CLOEXEC flag.

With kind regards,
Ivan

On 7/25/18 5:49 AM, Chris Hegarty wrote:

Alan,


On 25 Jul 2018, at 08:24, Alan Bateman  wrote:

...

As I said previously, the patch isn't complete so native code calling fork/exec 
may still have to deal with other file descriptors that are inherited into the 
child. I don't object to doing this in phases of course but somehow we have 
managed to get by for 20 years without this being an issue.

I added the following to the JIRA description to make this clear:

"This JIRA issue, by itself, does not completely resolve the problem
of native code calling fork/exec, it may still have to deal with other
file descriptors that are inherited by the child. Instead this JIRA
issue is targeted at the socket and channels areas only, other areas
should be tackled on a phased approach though separate JIRA
issues."


The updates to the various site to use the NET_* functions are fine. However, I 
think the new functions in net_util_md.c could be cleaner. I think it would be 
better to fallback to socket/socketpair + fcntl when the initial call fails 
with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
 int s;
#if defined(SOCK_CLOEXEC)
 s = socket(domain, type | SOCK_CLOEXEC, protocol);
 if (s != -1 || (s == -1 && errno != EINVAL)) {
 return s;
 }
#endif
 s = socket(domain, type, protocol);
 if (s != -1) {
 // Best effort - return value is intentionally ignored since the socket
 // was successfully created.
 fcntl(s, F_SETFD, FD_CLOEXEC);
 }
 return s;
}
---

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8207335/webrev.01/

-Chris.


--
With kind regards,
Ivan Gerasimov

Re: RFR : 8205959 : Do not restart close if errno is EINTR

[Baesken, Matthias]

Hello,   there was some discussion on the topic  in  the review of  
https://bugs.openjdk.java.net/browse/JDK-8206145
And  an outcome was that   at least   AIX recommends to repeat the close call 
on EINTR  .

Best regards, Matthias

> 
> Message: 4
> Date: Wed, 27 Jun 2018 18:23:36 -0500
> From: David Lloyd 
> To: Ivan Gerasimov 
> Cc: net-dev@openjdk.java.net
> Subject: Re: RFR : 8205959 : Do not restart close if errno is EINTR
> Message-ID: <7706c3a4-9f23-47a2-8196-0d0c40507...@redhat.com>
> Content-Type: text/plain; charset="us-ascii"
> 
> According to http://man7.org/linux/man-pages/man2/close.2.html it is
> currently platform-dependent whether close() must or must not (seems to
> be no middle ground) be retried.  Might have to do some #ifdef guarding?
> 
> --
> - DML
> 
> 
> > On Jun 27, 2018, at 6:15 PM, Ivan Gerasimov 
> wrote:
> >
> > Hello!
> >
> > When closing a socket via NET_SocketClose(int fd), a close(fd) is called.
> > The later is wrapped in a retry-loop, which is wrong because close() is not
> restartable.
> >
> > The `man 2 close` states:
> > """
> > ... close() should not be retried after an EINTR since this may cause a
> reused descriptor from another thread to be closed.
> > """
> >
> > Would you please help review a trivial fix?
> >
> > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8205959
> > WEBREV: http://cr.openjdk.java.net/~igerasim/8205959/00/webrev/
> >
> > Thanks in advance!
> >
> > --
> > With kind regards,
> > Ivan Gerasimov
> >
> -- next part --
> An HTML attachment was scrubbed...
> URL:  dev/attachments/20180627/07adf407/attachment.html>
> 

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 24/07/2018 21:34, Chris Hegarty wrote:

On 19 Jul 2018, at 18:41, Andrew Luo  wrote:

Just checking - is there any other changes that I should make to the patch, or 
anything else you guys need me to do?

A webrev genderated from Andrew’s patch along with:

1) some additional includes of “net_util_md.h” in several missing places
 in the jdk.net module’s source, as well as the appropriate make change:
EXTRA_HEADER_DIRS := \
   java.base:libnet,

2) simplified the ifdef structure for NET_Socket and NET_SocketPair
 in net_util_md.c, and some comment updates, to make it more
 readable.

http://cr.openjdk.java.net/~chegar/8207335/webrev.00/


Thanks for generating a webrev.

As I said previously, the patch isn't complete so native code calling 
fork/exec may still have to deal with other file descriptors that are 
inherited into the child. I don't object to doing this in phases of 
course but somehow we have managed to get by for 20 years without this 
being an issue.


The updates to the various site to use the NET_* functions are fine. 
However, I think the new functions in net_util_md.c could be cleaner. I 
think it would be better to fallback to socket/socketpair + fcntl when 
the initial call fails with EINVAL.


-Alan

Re: Connection timeouts in JEP 321 (HTTP Client)

[Michael McMahon]

Hi Markus,

The API and implementation supports an overall response timeout through 
the method:


HttpRequest.Builder timeout​(Duration duration)

So, I don't think any application should be required to wait for 130 
seconds.
It does not currently have a timeout setting specific for connection 
setup though.

Maybe that is something which could be added in a future release.

- Michael.


On 24/07/2018, 19:34, Markus Peloquin wrote:


Somebody pointed me at the upcoming HTTP client implementation, and 
I'm sad to see that connection timeouts are missing from the 
implementation (the old HTTP API). Is the absence of connection 
timeouts intended or an oversight? I'd like to see it added, and it 
looks like a simple change to me.


http://openjdk.java.net/jeps/321

http://mail.openjdk.java.net/pipermail/jdk-dev/2018-March/000996.html


There are some environments (such as AWS VPCs), where connection 
failures are only indicated by a connection timeout. This is because 
ICMP 'Destination Unreachable' packets are often not forwarded to the 
client (by load balancers, private links, etc) and there are 
supposedly some security concerns with allowing them by default. Those 
ICMP packets give immediate failures (net/host/protocol/port 
unreachable), but timeouts are slow.



The default timeout is unbounded in Java, though the TCP 
implementation of the OS times-out connection establishment to around 
130 seconds.



It looks like the implementation uses SocketChannel, which still 
supports timeouts via chan.socket().connect(addr, timeout).



Markus

Re: Connection timeouts in JEP 321 (HTTP Client)

[Bernd Eckenfels]

Hello,

I think the overall request Timeout is not a good substitute (if it applies to 
connect or DNS at all). Typically you want a small connect Timeout to allow 
failover or retry and a larger request Timeout to allow long running activities.

However propagating the request Timeout (shortened) to the default connect 
Timeout would be an additional improvement independent from the socket connect 
timeout.

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: -1022778048m Auftrag von
Gesendet: Mittwoch, Juli 25, 2018 11:00 AM
An: Markus Peloquin
Cc: net-dev@openjdk.java.net
Betreff: Re: Connection timeouts in JEP 321 (HTTP Client)

Hi Markus,

The API and implementation supports an overall response timeout through the 
method:

HttpRequest.Builder timeout​(Duration duration)

So, I don't think any application should be required to wait for 130 seconds.
It does not currently have a timeout setting specific for connection setup 
though.
Maybe that is something which could be added in a future release.

- Michael.


On 24/07/2018, 19:34, Markus Peloquin wrote:
Somebody pointed me at the upcoming HTTP client implementation, and I'm sad to 
see that connection timeouts are missing from the implementation (the old HTTP 
API). Is the absence of connection timeouts intended or an oversight? I'd like 
to see it added, and it looks like a simple change to me.
http://openjdk.java.net/jeps/321
http://mail.openjdk.java.net/pipermail/jdk-dev/2018-March/000996.html

There are some environments (such as AWS VPCs), where connection failures are 
only indicated by a connection timeout. This is because ICMP 'Destination 
Unreachable' packets are often not forwarded to the client (by load balancers, 
private links, etc) and there are supposedly some security concerns with 
allowing them by default. Those ICMP packets give immediate failures 
(net/host/protocol/port unreachable), but timeouts are slow.

The default timeout is unbounded in Java, though the TCP implementation of the 
OS times-out connection establishment to around 130 seconds.

It looks like the implementation uses SocketChannel, which still supports 
timeouts via chan.socket().connect(addr, timeout).

Markus

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Chris Hegarty]

Alan,

> On 25 Jul 2018, at 08:24, Alan Bateman  wrote:
> 
> ...
> 
> As I said previously, the patch isn't complete so native code calling 
> fork/exec may still have to deal with other file descriptors that are 
> inherited into the child. I don't object to doing this in phases of course 
> but somehow we have managed to get by for 20 years without this being an 
> issue.

I added the following to the JIRA description to make this clear:

"This JIRA issue, by itself, does not completely resolve the problem
of native code calling fork/exec, it may still have to deal with other
file descriptors that are inherited by the child. Instead this JIRA
issue is targeted at the socket and channels areas only, other areas
should be tackled on a phased approach though separate JIRA
issues."

> The updates to the various site to use the NET_* functions are fine. However, 
> I think the new functions in net_util_md.c could be cleaner. I think it would 
> be better to fallback to socket/socketpair + fcntl when the initial call 
> fails with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
int s;
#if defined(SOCK_CLOEXEC)
s = socket(domain, type | SOCK_CLOEXEC, protocol);
if (s != -1 || (s == -1 && errno != EINVAL)) {
return s;
}
#endif
s = socket(domain, type, protocol);
if (s != -1) {
// Best effort - return value is intentionally ignored since the socket
// was successfully created.
fcntl(s, F_SETFD, FD_CLOEXEC);
}
return s;
}
---

Updated webrev:
  http://cr.openjdk.java.net/~chegar/8207335/webrev.01/

-Chris.

Re: Connection timeouts in JEP 321 (HTTP Client)

[Chris Hegarty]

Clearly the request builder `timeout` method can be used to avoid
extremely long connection timeouts ( as demonstrated below ), but I see
Bernd's call for more fine grained control over various aspects of the
request.

I'm not opposed to an "HttpRequest.Builder.connectTimeout` method, but
this is coming very late in the JDK 11 development cycle. How important
is this for 11, given that the naked `timeout` can be used, somewhat, to
mitigate against long connection timeouts?


$ cat Get.java

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.net.http.HttpResponse.BodyHandlers;
import java.time.Duration;

public class Get {
  public static void main(String[] args) throws Exception {
long before = System.nanoTime();
try {
  HttpClient client = HttpClient.newHttpClient();
  HttpRequest request = HttpRequest.newBuilder(URI.create(args[0]))
.timeout(Duration.ofSeconds(10))
.build();
  HttpResponse response = client.send(request, 
BodyHandlers.ofString());
  System.out.println("response :" + response);
} finally {
  System.out.println("elapsed secs :" + ((System.nanoTime() - 
before)/1000_000_000));
}
  }
}
$ javac Get.java
$ java Get http://example.com:81
elapsed secs :10
Exception in thread "main" java.net.http.HttpTimeoutException: request timed out
   at 
java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:551)
   at 
java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:113)
   at Get.main(Get.java:17)

-Chris.

> On 24 Jul 2018, at 19:34, Markus Peloquin  wrote:
> 
> Somebody pointed me at the upcoming HTTP client implementation, and I'm sad 
> to see that connection timeouts are missing from the implementation (the old 
> HTTP API). Is the absence of connection timeouts intended or an oversight? 
> I'd like to see it added, and it looks like a simple change to me.
> http://openjdk.java.net/jeps/321
> http://mail.openjdk.java.net/pipermail/jdk-dev/2018-March/000996.html
> 
> There are some environments (such as AWS VPCs), where connection failures are 
> only indicated by a connection timeout. This is because ICMP 'Destination 
> Unreachable' packets are often not forwarded to the client (by load 
> balancers, private links, etc) and there are supposedly some security 
> concerns with allowing them by default. Those ICMP packets give immediate 
> failures (net/host/protocol/port unreachable), but timeouts are slow.
> 
> The default timeout is unbounded in Java, though the TCP implementation of 
> the OS times-out connection establishment to around 130 seconds.
> 
> It looks like the implementation uses SocketChannel, which still supports 
> timeouts via chan.socket().connect(addr, timeout).
> 
> Markus

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 25/07/2018 13:49, Chris Hegarty wrote:

:

The updates to the various site to use the NET_* functions are fine. However, I 
think the new functions in net_util_md.c could be cleaner. I think it would be 
better to fallback to socket/socketpair + fcntl when the initial call fails 
with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
 int s;
#if defined(SOCK_CLOEXEC)
 s = socket(domain, type | SOCK_CLOEXEC, protocol);
 if (s != -1 || (s == -1 && errno != EINVAL)) {
 return s;
 }
#endif
 s = socket(domain, type, protocol);
 if (s != -1) {
 // Best effort - return value is intentionally ignored since the socket
 // was successfully created.
 fcntl(s, F_SETFD, FD_CLOEXEC);
 }
 return s;
}
---

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8207335/webrev.01/


This looks much better - thanks!

-Alan

Re: Connection timeouts in JEP 321 (HTTP Client)

[Markus Ivan Peloquin]

It's unfortunate that the user sees this when a connection wasn't even 
established:

java.net.http.HttpTimeoutException: request timed out

Could it use Channel::isOpen to return a more specific exception? 
HttpConnectionException? Or at least a more specific message.


> How important is this for 11 ...?
If the question's for me, it's a strong preference. I would prefer to 
keep my legacy code in place in order to preserve my short connection 
timeouts and the more specific SocketTimeoutException root cause. I have 
to diagnose problems using backtraces, and the lack of information makes 
it difficult. Though I may be okay with something like 
HttpConnectionException.


Markus

On 2018-07-25 07:43, Chris Hegarty wrote:

Clearly the request builder `timeout` method can be used to avoid
extremely long connection timeouts ( as demonstrated below ), but I see
Bernd's call for more fine grained control over various aspects of the
request.

I'm not opposed to an "HttpRequest.Builder.connectTimeout` method, but
this is coming very late in the JDK 11 development cycle. How important
is this for 11, given that the naked `timeout` can be used, somewhat, to
mitigate against long connection timeouts?


$ cat Get.java

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.net.http.HttpResponse.BodyHandlers;
import java.time.Duration;

public class Get {
   public static void main(String[] args) throws Exception {
 long before = System.nanoTime();
 try {
   HttpClient client = HttpClient.newHttpClient();
   HttpRequest request = HttpRequest.newBuilder(URI.create(args[0]))
 .timeout(Duration.ofSeconds(10))
 .build();
   HttpResponse response = client.send(request, 
BodyHandlers.ofString());
   System.out.println("response :" + response);
 } finally {
   System.out.println("elapsed secs :" + ((System.nanoTime() - 
before)/1000_000_000));
 }
   }
}
$ javac Get.java
$ java Get http://example.com:81
elapsed secs :10
Exception in thread "main" java.net.http.HttpTimeoutException: request timed out
at 
java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:551)
at 
java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:113)
at Get.main(Get.java:17)

-Chris.


On 24 Jul 2018, at 19:34, Markus Peloquin  wrote:

Somebody pointed me at the upcoming HTTP client implementation, and I'm sad to 
see that connection timeouts are missing from the implementation (the old HTTP 
API). Is the absence of connection timeouts intended or an oversight? I'd like 
to see it added, and it looks like a simple change to me.
http://openjdk.java.net/jeps/321
http://mail.openjdk.java.net/pipermail/jdk-dev/2018-March/000996.html

There are some environments (such as AWS VPCs), where connection failures are 
only indicated by a connection timeout. This is because ICMP 'Destination 
Unreachable' packets are often not forwarded to the client (by load balancers, 
private links, etc) and there are supposedly some security concerns with 
allowing them by default. Those ICMP packets give immediate failures 
(net/host/protocol/port unreachable), but timeouts are slow.

The default timeout is unbounded in Java, though the TCP implementation of the 
OS times-out connection establishment to around 130 seconds.

It looks like the implementation uses SocketChannel, which still supports 
timeouts via chan.socket().connect(addr, timeout).

Markus