Re: [PATCH] SOCK_CLOEXEC for opening sockets

2018-07-25 Thread 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

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

2018-07-25 Thread 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:

Re: [PATCH] SOCK_CLOEXEC for opening sockets

2018-07-25 Thread 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

Re: Connection timeouts in JEP 321 (HTTP Client)

2018-07-25 Thread 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

Re: Connection timeouts in JEP 321 (HTTP Client)

2018-07-25 Thread 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)

Re: [PATCH] SOCK_CLOEXEC for opening sockets

2018-07-25 Thread 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

Re: Connection timeouts in JEP 321 (HTTP Client)

2018-07-25 Thread 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

Re: [PATCH] SOCK_CLOEXEC for opening sockets

2018-07-25 Thread 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.

Re: Connection timeouts in JEP 321 (HTTP Client)

2018-07-25 Thread 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