Re: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

[Alan Bateman]

I'm sure Michael will look at this but I have a question - shouldn't 
SocketImpl throw UOE for this case? I'm just wondering if checking the 
supported options in setOption/getOption is just covering up an issue 
with the SocketImpl methods.


-Alan


On 08/12/2015 12:36, Svetlana Nikandrova wrote:

Little reminder.

On 03.12.2015 16:06, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and 
setOption sockets' methods similar way as it was done prior jdk 9 in 
Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. I 
added test to the ProblemList.


Thank you,
Svetlana

Re: RFR [9] 8143554: UnsupportedOperationException is not thrown for unsupported options

[Svetlana Nikandrova]

Hi Alan,

thank you for your replay. Please let me explain a little.
For example ServerSocket and Socket have different supported options 
set, but the same SocketImpl under the hood.
Yes, SocketImpl's setOptions() and getOptions() can be modified to add 
additional check for the actual socket type that encloses that socket 
implementation, but I believe this will intricate method's logic and 
tangle the dependencies. I like it how it was done in jdk 8 - clean and 
simple - so I think it's a good idea to maintain that approach in jdk 9 
as well.


Thank you,
Svetlana

On 08.12.2015 15:56, Alan Bateman wrote:


I'm sure Michael will look at this but I have a question - shouldn't 
SocketImpl throw UOE for this case? I'm just wondering if checking the 
supported options in setOption/getOption is just covering up an issue 
with the SocketImpl methods.


-Alan


On 08/12/2015 12:36, Svetlana Nikandrova wrote:

Little reminder.

On 03.12.2015 16:06, Svetlana Nikandrova wrote:

Hello,

please review a simple fix for:
https://bugs.openjdk.java.net/browse/JDK-8143554

See webrev here:
http://cr.openjdk.java.net/~kshefov/8143554/webrev.00/ 



Fix added explicit check for option support to getOption and 
setOption sockets' methods similar way as it was done prior jdk 9 in 
Sockets class.
Fix also exposed another problem with sockets' supported options: 
JDK-8143923  that 
cause new test failure test/java/net/SocketOption/OptionsTest.java. 
I added test to the ProblemList.


Thank you,
Svetlana

Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly

[Rob McKenna]

Testing has shown that when a timeout < 1000ms is specified the 
IcmpSendEcho calls fail (apparently) randomly. Once the timeout is 
1000ms or greater it works as expected. Therefore I've updated the fix 
to use 1000ms as a minimum. The existing logic ensures that the ttl is 
less than the specified timeout in any case:


http://cr.openjdk.java.net/~robm/8143397/webrev.02/

-Rob

On 01/12/15 14:59, Rob McKenna wrote:

It appears that there is an undocumented minimum timeout in the
IcmpSendEcho* functions. If the timeout parameter is set to a number
below this minimum timeout it is effectively ignored. Thus if you wanted
to ensure that you could ping a particular host within a certain timeout
less than the undocumented minimum, you could potentially receive a
false positive. (i.e. if you set the timeout to 20ms but the ping takes
30ms, IcmpSendEcho will still succeed)

The following fix checks the icmp reply packet and compares the round
trip time to the requested timeout parameter before deciding whether the
call was successful or not:

http://cr.openjdk.java.net/~robm/8143397/webrev.01/

 -Rob

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Thank you very much for your help, Alan!

Thanks,
Lucy

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com] 
Sent: Tuesday, December 08, 2015 8:46 AM
To: Lu, Yingqi ; Michael McMahon 
; Volker Simonis 
Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net; 
Kharbas, Kishor ; Viswanathan, Sandhya 
; Aundhe, Shirish 
Subject: Re: Patch for adding SO_REUSEPORT socket option



On 08/12/2015 00:53, Lu, Yingqi wrote:
> Hi Alan,
>
> I heard that the feature freeze for OpenJDK9 is approaching. Given the 
> significant performance impact (up to 1.93x with Hadoop Distributed File 
> System) this feature provides and the status of the existing work, we would 
> be really interested in see this feature to be approved for OpenJDK9. We will 
> surely continue working with the community to address opens/issues until it 
> is completely ready to be merged into the source code tree.

There is a proposal to move out JDK 9 by 6 months [1]. In any case, I think we 
are close to agreement on this socket option and I will reply to your others 
mail the implementation.

-Alan.

[1]
http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-December/003149.html

Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly

[Xuelei Fan]

On 12/9/2015 9:44 AM, Rob McKenna wrote:
> The intention of the 2nd revision of the fix is to make the undocumented
> 1000ms problem a non issue.
> 
> If a user calls this function with a timeout of 200ms that timeout is
> automatically substituted for 1000ms in the IcmpSendEcho call. Once the
> response is received its RTT is checked to make sure it is lower than
> 200ms. If not the method returns false.
> 
OK.  It makes sense to me.

Thanks,
Xuelei

> That being the case though the call could potentially take up to 1000ms
> to complete where a host is not reachable even though a smaller timeout
> is specified. The return value of isReachable() will correctly say
> whether the host was reachable or not within the actual timeout
> specified by the user however.
> 
> In order to avoid the older (and less reliable) way of testing
> reachability I feel that this small corner case is a worthwhile tradeoff.
> 
> -Rob
> 
> On 09/12/15 01:31, Xuelei Fan wrote:
>> Is it nice to say in the spec that it is not reliable if the timeout is
>> too small?  At least 1000ms timeout by default may be not acceptable in
>> some circumstances.
>>
>> Xuelei
>>
>> On 12/9/2015 12:31 AM, Rob McKenna wrote:
>>> Testing has shown that when a timeout < 1000ms is specified the
>>> IcmpSendEcho calls fail (apparently) randomly. Once the timeout is
>>> 1000ms or greater it works as expected. Therefore I've updated the fix
>>> to use 1000ms as a minimum. The existing logic ensures that the ttl is
>>> less than the specified timeout in any case:
>>>
>>> http://cr.openjdk.java.net/~robm/8143397/webrev.02/
>>>
>>>  -Rob
>>>
>>> On 01/12/15 14:59, Rob McKenna wrote:
 It appears that there is an undocumented minimum timeout in the
 IcmpSendEcho* functions. If the timeout parameter is set to a number
 below this minimum timeout it is effectively ignored. Thus if you
 wanted
 to ensure that you could ping a particular host within a certain
 timeout
 less than the undocumented minimum, you could potentially receive a
 false positive. (i.e. if you set the timeout to 20ms but the ping takes
 30ms, IcmpSendEcho will still succeed)

 The following fix checks the icmp reply packet and compares the round
 trip time to the requested timeout parameter before deciding whether
 the
 call was successful or not:

 http://cr.openjdk.java.net/~robm/8143397/webrev.01/

   -Rob
>>

Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly

[Mark Sheppard]

Hi Rob,
change looks fine and handles the MS idiosyncrasies neatly
change works fine ... consistent responses and failing test returns 
expected results



regards
Mark

On 09/12/2015 01:44, Rob McKenna wrote:
The intention of the 2nd revision of the fix is to make the 
undocumented 1000ms problem a non issue.


If a user calls this function with a timeout of 200ms that timeout is 
automatically substituted for 1000ms in the IcmpSendEcho call. Once 
the response is received its RTT is checked to make sure it is lower 
than 200ms. If not the method returns false.


That being the case though the call could potentially take up to 
1000ms to complete where a host is not reachable even though a smaller 
timeout is specified. The return value of isReachable() will correctly 
say whether the host was reachable or not within the actual timeout 
specified by the user however.


In order to avoid the older (and less reliable) way of testing 
reachability I feel that this small corner case is a worthwhile tradeoff.


-Rob

On 09/12/15 01:31, Xuelei Fan wrote:

Is it nice to say in the spec that it is not reliable if the timeout is
too small?  At least 1000ms timeout by default may be not acceptable in
some circumstances.

Xuelei

On 12/9/2015 12:31 AM, Rob McKenna wrote:

Testing has shown that when a timeout < 1000ms is specified the
IcmpSendEcho calls fail (apparently) randomly. Once the timeout is
1000ms or greater it works as expected. Therefore I've updated the fix
to use 1000ms as a minimum. The existing logic ensures that the ttl is
less than the specified timeout in any case:

http://cr.openjdk.java.net/~robm/8143397/webrev.02/

 -Rob

On 01/12/15 14:59, Rob McKenna wrote:

It appears that there is an undocumented minimum timeout in the
IcmpSendEcho* functions. If the timeout parameter is set to a number
below this minimum timeout it is effectively ignored. Thus if you 
wanted
to ensure that you could ping a particular host within a certain 
timeout

less than the undocumented minimum, you could potentially receive a
false positive. (i.e. if you set the timeout to 20ms but the ping 
takes

30ms, IcmpSendEcho will still succeed)

The following fix checks the icmp reply packet and compares the round
trip time to the requested timeout parameter before deciding 
whether the

call was successful or not:

http://cr.openjdk.java.net/~robm/8143397/webrev.01/

  -Rob

Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly

[Rob McKenna]

The intention of the 2nd revision of the fix is to make the undocumented 
1000ms problem a non issue.


If a user calls this function with a timeout of 200ms that timeout is 
automatically substituted for 1000ms in the IcmpSendEcho call. Once the 
response is received its RTT is checked to make sure it is lower than 
200ms. If not the method returns false.


That being the case though the call could potentially take up to 1000ms 
to complete where a host is not reachable even though a smaller timeout 
is specified. The return value of isReachable() will correctly say 
whether the host was reachable or not within the actual timeout 
specified by the user however.


In order to avoid the older (and less reliable) way of testing 
reachability I feel that this small corner case is a worthwhile tradeoff.


-Rob

On 09/12/15 01:31, Xuelei Fan wrote:

Is it nice to say in the spec that it is not reliable if the timeout is
too small?  At least 1000ms timeout by default may be not acceptable in
some circumstances.

Xuelei

On 12/9/2015 12:31 AM, Rob McKenna wrote:

Testing has shown that when a timeout < 1000ms is specified the
IcmpSendEcho calls fail (apparently) randomly. Once the timeout is
1000ms or greater it works as expected. Therefore I've updated the fix
to use 1000ms as a minimum. The existing logic ensures that the ttl is
less than the specified timeout in any case:

http://cr.openjdk.java.net/~robm/8143397/webrev.02/

 -Rob

On 01/12/15 14:59, Rob McKenna wrote:

It appears that there is an undocumented minimum timeout in the
IcmpSendEcho* functions. If the timeout parameter is set to a number
below this minimum timeout it is effectively ignored. Thus if you wanted
to ensure that you could ping a particular host within a certain timeout
less than the undocumented minimum, you could potentially receive a
false positive. (i.e. if you set the timeout to 20ms but the ping takes
30ms, IcmpSendEcho will still succeed)

The following fix checks the icmp reply packet and compares the round
trip time to the requested timeout parameter before deciding whether the
call was successful or not:

http://cr.openjdk.java.net/~robm/8143397/webrev.01/

  -Rob

RE: Patch for adding SO_REUSEPORT socket option

[Lu, Yingqi]

Hi All,

Here is an update on the progress. Based on the comments and feedback we 
received, we modified following items in the upcoming version of the patch 
(will be version #6). We are in the process of testing. I will be on vacation 
from Dec. 9th and will be back to office on Dec. 17th. We will hopefully be 
able to send out version #6 of the patch on Dec. 18th. 

1. setReusePort and getReusePort methods are removed from 
Socket/ServerSocket/DatagramSocket.

2. Minor wording changes to StandardSocketOptions regarding to SO_REUSEPORT. 
This include adding "usually" to the sentences and also including @since 9 at 
the end.

3. inside sun.nio.ch.Net,
 3.1 Change Net.reuseportSupported to Net.isReusePortAvailable and make it 
as private. 
 3.2 The native implantation isReusePortAvailable0 is done inside Net.c 
which points to a newly added native function named reuseport_available(). 
reuseport_available() is added in 
./jdk/src/java.base/share/native/libnet/net_util.c.

4. inside java.net,
 4.1 Add a function called isReusePortAvailable in 
./jdk/src/java.base/share/classes/java/net/SocketImpl.java. Create SocketImpl.c 
file for both UNIX and Windows to add the native implementation named as 
isReusePortAvailable0. 
 4.2 The native implementation points to the same reuseport_available() 
described in 3.2

5. Similarly, inside jdk.net, add isReusePortAvailable and its native 
implementation.

6. Inside ./jdk/src/java.base/unix/native/libnet/net_util_md.c, add a new 
function called reuseport_supported(). This is the function that calls 
setsockopt and check for SO_REUSEPORT. reuseport_available() points to 
reuseport_supported().

7. reuseport_supported() is called during JNI_OnLoad function and the result is 
also the return value of reuseport_available(). This makes SO_REUSEPORT check 
is done once at the beginning and the results will be cached for later use.

8. #3-#7 follows the example of the IPv6 check. Please let me know if there is 
anything I missed.

9. Removed setting SO_REUSEPORT for debugger agent

10. For all the tests, instead of calling Net.reuseportSupport method, use 
supportedOptions to check for SO_REUSEPORT. 

11. Remove test/java/net/Socket/setReusePort/Basic.java since the setReusePort 
and getReusePort methods are removed.

12. Setting SO_REUSEPORT value to platform dependent instead of using 15 for 
everyone.

13. Make SO_REUSEPORT to be disabled by default for both 
ServerSocketChannelImpl and SocketChannelImpl. Leave the feature enablement to 
the users.

14. Updated the copyright date.

Thanks,
Lucy

-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Lu, Yingqi
Sent: Tuesday, December 08, 2015 9:11 AM
To: Alan Bateman ; Michael McMahon 
; Volker Simonis 
Cc: Viswanathan, Sandhya ; 
net-dev@openjdk.java.net; Kharbas, Kishor ; Aundhe, 
Shirish ; Kaczmarek, Eric 
Subject: RE: Patch for adding SO_REUSEPORT socket option

Thank you very much for your help, Alan!

Thanks,
Lucy

-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com] 
Sent: Tuesday, December 08, 2015 8:46 AM
To: Lu, Yingqi ; Michael McMahon 
; Volker Simonis 
Cc: Kaczmarek, Eric ; net-dev@openjdk.java.net; 
Kharbas, Kishor ; Viswanathan, Sandhya 
; Aundhe, Shirish 
Subject: Re: Patch for adding SO_REUSEPORT socket option



On 08/12/2015 00:53, Lu, Yingqi wrote:
> Hi Alan,
>
> I heard that the feature freeze for OpenJDK9 is approaching. Given the 
> significant performance impact (up to 1.93x with Hadoop Distributed File 
> System) this feature provides and the status of the existing work, we would 
> be really interested in see this feature to be approved for OpenJDK9. We will 
> surely continue working with the community to address opens/issues until it 
> is completely ready to be merged into the source code tree.

There is a proposal to move out JDK 9 by 6 months [1]. In any case, I think we 
are close to agreement on this socket option and I will reply to your others 
mail the implementation.

-Alan.

[1]
http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-December/003149.html