RFR:8194298 Add support for per Socket configuration of TCP keepalive

[Ivan Gerasimov]

Hi Vyom!

A few random comments:

1)

src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java

Collections.emptySet() produces an immutable set, so it must not be 
possible to add elements to it.

Is there a test that verifies that ExtendedSocketOption.options(short) 
works as expected?

2)

In several files:

Personally, I found it counter-intuitive that static 
ExtendedSocketOptions.getInstance() is imported and used without the 
class name.

It is hard to realize from the context to which class the getInstance() 
call belongs to.

3)

src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c

Formatting nits:  Please add a space after comma at line 125, add an 
empty line after 128.

4)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java

Please fix formatting at lines 355-358 (spaces, else after })

Can the new set/get methods at 305-325 be declared private?

5)
src/jdk.net/share/classes/jdk/net/Sockets.java

It is probably a matter of preference, but it might be slightly more 
efficient to call set.add() three times instead of calling 
set.addAll(Set.of(...)).

Similarly, it may be a little bit faster to do (set.contains(A) && 
set.contains(B) && set.contains(C)) instead of set.containsAll(Set.of(A, 
B, C)).

6)
test/jdk/java/nio/channels/AsynchronousServerSocketChannel/Basic.java

indentation is broken at lines 176-183

7)
test/jdk/java/nio/channels/AsynchronousSocketChannel/Basic.java

Extra space in the indentation at 192 :)

8)
test/jdk/java/nio/channels/SocketChannel/SocketOptionTests.java

Would it make sense to add a test that either all three options 
{TCP_KEEPCOUNT, TCP_KEEPIDLE, TCP_KEEPINTERVAL} are supported or none of 
them?

With kind regards,
Ivan


On 5/10/18 8:21 AM, vyom tewari wrote:
> Hi,
>
> Please find the latest 
> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html). 
> I incorporated most of the review comments. Chris as you suggested in 
> below mail i did not added the note for upper-bound because values are 
> also OS specific.
>
> Thanks,
>
> Vyom
>
>
> On Monday 23 April 2018 07:26 PM, Chris Hegarty wrote:
>>
>> On 23/04/18 13:34, Alan Bateman wrote:
>>> On 23/04/2018 13:05, vyom tewari wrote:
>>>> Hi,
>>>>
>>>> Please find the latest 
>>>> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
>>>> I incorporated  most of the review comments.
>>> This looks much better but I think the second paragraph of the spec 
>>> of each option needs to be inverted so that it states clearly what 
>>> the options does before it gets into the background of SO_KEEPALIVE. 
>>> For example, TCP_KEEPALIVE should say that it sets the idle period 
>>> for keep alive before it explains SO_KEEPALIVE. 
>>
>> Mea culpa, I ordered the paragraphs as they are to be consistent
>> with TCP_QUICKACK. I don't have any objection if they are reversed.
>>
>>> The currently wording also begs questions as to whether the socket 
>>> option means anything when SO_KEEPALIVE is not enabled.
>>
>> It's implicit. The option can still be set and its value retrieved.
>>
>>
>>> Also it probably should say something about whether it can be 
>>> changed at any time or not.
>>
>> You can set it any time. Of course, it may not be effective
>> immediately, depending on the exact state of the socket.
>>
>> We may also want to add a note that the accepted values may
>> have an upper-bound. For example, the following is the largest
>> set of values that I can set on my Ubuntu Linux, without an
>> exception being thrown [*].
>>
>>  TCP_KEEPIDLE = 32767
>>  TCP_KEEPINTERVAL = 32767
>>  TCP_KEEPCOUNT = 127
>>
>> -Chris.
>>
>> [*] "java.net.SocketException: Invalid argument" when a given
>> value is "too" large.
>
>

-- 
With kind regards,
Ivan Gerasimov