RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Alan,

thanks for pointing out, i am forwarding it to net-dev list.

Vyom


On Thursday 12 October 2017 03:54 PM, Alan Bateman wrote:
> Best to reply on net-dev as that is where the main review should be 
> going on (seems there are at two review threads going, maybe they 
> could unite on net-dev).
>
>
> On 12/10/2017 10:01, vyom tewari wrote:
>> Hi Roger,
>>
>> Thanks for the review, i incorporated all review comments from you 
>> except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
>> the option to omit without
>>  embedding the name."). ExtendedSocketOptions.java is part of 
>> "jdk.net"  so we can not directly use it in java.base, hence i used 
>> the option name to filter "TCP_QUICKACK".
>>
>> Please find the update 
>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).
>>
>> Thanks,
>>
>> Vyom
>>
>>
>> On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
>>> Hi Vyom,
>>>
>>> Comments:
>>>
>>>  - update copyright
>>>  - use @since 18.3 instead of @since 10
>>>
>>> - ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in 
>>> the exception messages.
>>>
>>>     Line 144: if you are going to keep the assert, add a explanation 
>>> about how it could happen.
>>>     I'd remove the assert.
>>>
>>>  - The first sentence should be a complete sentence: "TCP_QUICKACK 
>>> socket option enables sending TCP/IP acks immediately" or similar.
>>>
>>>  - A reference to the appropriate TCP/IP spec for quickack would be 
>>> a good addition. At least the RFC # and section.
>>>  - 85: space after "."  The phrasing is a bit odd in places in the 
>>> javadoc.
>>>  - line 81/82 the value is true to enable and false to disable.
>>>  - This phrase is confusing: "it only enables a switch to or from 
>>> TCP_QUICKACK mode";
>>>    Since it is set on a socket, it should remain set on that socket 
>>> until it is changed.
>>>
>>>  - 203: be consistent in using enable/disable in parameters, etc 
>>> even for private methods.
>>>     "on" -> "enable"
>>>
>>> PlainDatagramSocketImpl: 89:
>>>   Why create a new set with zero or one items just to throw it away?
>>>   Use the iterator to add only the non-TCP_QUICKACK options to the 
>>> supported options.
>>>  Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
>>> the option to omit without
>>>  embedding the name.
>>>
>>>
>>> Sockets.java:
>>>   - The initialization of isQuickAckAvailable might be cleaner as an 
>>> nested static class
>>>     that initializes the value in its static initializer. That would 
>>> delay the init til needed
>>>     and avoid extra flags.
>>>
>>> LinuxSocketOptions.java:
>>>    - the native methods should be static; since the instance is unused.
>>>
>>>  - line 41: the return type should be Boolean
>>>
>>>  - line 46: the return type of getQuickAck0 should be Boolean like 
>>> the argument to set.
>>>
>>>  - line 34:  using JNU_ThrowByNameWithLastError directly is 
>>> sufficient; if the errno does not have a string unix supplies 
>>> "unknown error nnn".
>>>
>>>
>>> Regards, Roger
>>>
>>> On 10/10/2017 2:58 PM, Chris Hegarty wrote:
>>>> Vyom,
>>>>
>>>> What about suggestion 1) below, the name of the socket option?
>>>>
>>>> -Chris.
>>>>
>>>>> On 27 Sep 2017, at 09:56, vyom tewari <vyom.tewari at oracle.com> wrote:
>>>>>
>>>>> Hi Chris,
>>>>>
>>>>> Thanks for review, please find the latest 
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). 
>>>>> I incorporated review comments from you and re-base the patch to 
>>>>> our consolidated repo(jdk10/master).
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Vyom
>>>>>
>>>>>
>>>>> On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
>>>>>> Vyom,
>>>>>>
>>>>>>> On 11 Sep 2017, at 16:38, vyom tewari <vyom.tewari at oracle.com> 
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>> As jdk.net API already moved out of java.base, Please review the 
>>>>>>> below code change for jdk10.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
>>>>>>> Webrev: 
>>>>>>> http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
>>>>>>>
>>>>>> This looks quite good. Some comments:
>>>>>>
>>>>>> 1) I wonder if we should just call the option TCP_QUICKACK, 
>>>>>> rather than SO_QUICKACK? Similar to TCP_NODELAY.
>>>>>>
>>>>>> 2) I think LinuxSocketOptions.h is largely unnecessary, if we do 
>>>>>> 1) above.
>>>>>>
>>>>>> 3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, 
>>>>>> rather than jobject, thus avoiding the need for createBoolean. 
>>>>>> The conversation can happen in the Java layer.  Can you please 
>>>>>> reduce the lint length in this file, by wrapping similar to the 
>>>>>> style of the Solaris version.
>>>>>>
>>>>>> 4) ExtendedSocketOptions.java
>>>>>>    - drop the <p>, they are unnecessary.
>>>>>>    - I think, similar to TCP_NODELAY, the spec should say that 
>>>>>> the options is TCP specific. From TCP_NODELAY: "The socket option 
>>>>>> is specific to stream-oriented sockets using the TCP/IP protocol.”
>>>>>>    - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>
>>>
>>
>