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

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.6/index.html). 
I renamed the macosx files to 
"MacOSXSocketOptions.java".


Our internal tests are clean.

Thanks,
Vyom

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


-Alan.

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

[vyom tewari]

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


sure, i will rename the files to 
MacOSXSocketOptions.java I will send the modified 
webrev soon. Please do let me know if any buddy have better name 
suggestion than this.


Thanks,
vyom

-Alan.

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

[Alan Bateman]

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so I 
assume the final points around the implementation and naming of the JDK 
internal classes can be sorted out while the CSR is in progress.


-Alan.

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

[Langer, Christoph]

Hi,

As I had written previously, I’m still unclear whether the naming of the MacOS 
implementation files is the best choice.

Currently it is:
src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c

Maybe it  should rather be:
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c

Since for Linux we have:
src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c

I’m asking because I’m planning to add some AIX options and will have to choose 
a name for this implementation eventually.

@Alan: What do you think?

Best regards
Christoph


From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of vyom tewari
Sent: Montag, 14. Mai 2018 17:31
To: Alan Bateman <alan.bate...@oracle.com>; OpenJDK Network Dev list 
<net-dev@openjdk.java.net>
Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP 
keepalive


Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.5/index.html). Only 
change with the previous wrev(04) is i removed "socket type" as Alan suggested 
and used the default  constructor (Set<SocketOption> options = new 
HashSet<>();) in ExtendedSocketOptions.java

Thanks,

Vyom

On Sunday 13 May 2018 12:37 PM, Alan Bateman wrote:
On 12/05/2018 10:21, vyom tewari wrote:

:

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html<http://cr.openjdk.java.net/%7Evtewari/8194298/webrev0.4/index.html>).
I've skimmed through this webrev.

The spec for the new options mostly look good but all three include "The exact 
semantics of this socket option are socket type and system dependent". I assume 
"socket type" should be dropped from this as these are TCP options. Maybe you 
can borrow text from the TCP_NODELAY option where it has the statement "The 
socket option is specific to stream-oriented sockets using the TCP/IP protocol".

The changes to the NetworkChannel implementations look okay. The changes to the 
NIO tests looks okay too but would be good if you could keep the formatting 
consistent with the existing code if you can.

Ivan had good comments so I didn't spent as much time on that code and it seems 
very reviewed.

-Alan.

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

[Alan Bateman]

On 14/05/2018 16:30, vyom tewari wrote:


Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.5/index.html). 
Only change with the previous wrev(04) is i removed "socket type" as 
Alan suggested and used the default  constructor (Set 
options = new HashSet<>();) in ExtendedSocketOptions.java



This looks good to me. Not strictly required, but we adjust the wording 
for TCK_QUICKACK to remove "socket type" too.


-Alan

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

[vyom tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.5/index.html). 
Only change with the previous wrev(04) is i removed "socket type" as 
Alan suggested and used the default  constructor (Set 
options = new HashSet<>();) in ExtendedSocketOptions.java


Thanks,

Vyom


On Sunday 13 May 2018 12:37 PM, Alan Bateman wrote:

On 12/05/2018 10:21, vyom tewari wrote:

:

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).

I've skimmed through this webrev.

The spec for the new options mostly look good but all three include 
"The exact semantics of this socket option are socket type and system 
dependent". I assume "socket type" should be dropped from this as 
these are TCP options. Maybe you can borrow text from the TCP_NODELAY 
option where it has the statement "The socket option is specific to 
stream-oriented sockets using the TCP/IP protocol".


The changes to the NetworkChannel implementations look okay. The 
changes to the NIO tests looks okay too but would be good if you could 
keep the formatting consistent with the existing code if you can.


Ivan had good comments so I didn't spent as much time on that code and 
it seems very reviewed.


-Alan.

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

[Alan Bateman]

On 12/05/2018 10:21, vyom tewari wrote:

:

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).

I've skimmed through this webrev.

The spec for the new options mostly look good but all three include "The 
exact semantics of this socket option are socket type and system 
dependent". I assume "socket type" should be dropped from this as these 
are TCP options. Maybe you can borrow text from the TCP_NODELAY option 
where it has the statement "The socket option is specific to 
stream-oriented sockets using the TCP/IP protocol".


The changes to the NetworkChannel implementations look okay. The changes 
to the NIO tests looks okay too but would be good if you could keep the 
formatting consistent with the existing code if you can.


Ivan had good comments so I didn't spent as much time on that code and 
it seems very reviewed.


-Alan.

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

[vyom tewari]

On Saturday 12 May 2018 04:14 PM, Ivan Gerasimov wrote:

Thanks Vyom!

I like it much better now.

The last minorish comment:

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set options = new HashSet<>(5);

Please note that the constructor of HashSet wants the initial capacity 
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 
0.75 = 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be 
rounded up to 8 anyways.


thanks for detail explanation :) , i did not saw the code but i was 
under impression that it will use size as the nearest prime number for 
uniform distribution of elements in general.


However, I would recommend using the default constructor just to avoid 
confusion.
Or, alternatively, collect the options to an ArrayList of desired 
capacity and then make unmodifiable set with Set.copyOf(list).


No further comments from my side :)
if no further comment from others as well, i will change it to use 
default constructor at time of pushing.


Thanks,
Vyom



With kind regards,
Ivan


On 5/12/18 2:21 AM, vyom tewari wrote:

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

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

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


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

Was it intentional that when flowSupported == true, 
quickAckSupported == false, keepAliveOptSupported == true, the 
TCP_KEEP* options are *not* added to the resulting set?


If not, then can this set population be organized in more linear 
way:  Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman  
wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very 
generic method method name. As I mentioned previously, you could 
simplify all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

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

[Ivan Gerasimov]

Thanks Vyom!

I like it much better now.

The last minorish comment:

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set options = new HashSet<>(5);

Please note that the constructor of HashSet wants the initial capacity 
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 0.75 
= 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be 
rounded up to 8 anyways.


However, I would recommend using the default constructor just to avoid 
confusion.
Or, alternatively, collect the options to an ArrayList of desired 
capacity and then make unmodifiable set with Set.copyOf(list).


No further comments from my side :)

With kind regards,
Ivan


On 5/12/18 2:21 AM, vyom tewari wrote:

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

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

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
.filter((option) -> !option.name().startsWith("TCP_"))
.collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


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

Was it intentional that when flowSupported == true, quickAckSupported 
== false, keepAliveOptSupported == true, the TCP_KEEP* options are 
*not* added to the resulting set?


If not, then can this set population be organized in more linear 
way:  Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman  
wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very generic 
method method name. As I mentioned previously, you could simplify 
all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
static Set options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.












--
With kind regards,
Ivan Gerasimov

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

[vyom tewari]

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

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

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


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

Was it intentional that when flowSupported == true, quickAckSupported 
== false, keepAliveOptSupported == true, the TCP_KEEP* options are 
*not* added to the resulting set?


If not, then can this set population be organized in more linear way:  
Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman  wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very generic 
method method name. As I mentioned previously, you could simplify 
all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

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

[Ivan Gerasimov]

Hi Vyom!

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

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
.filter((option) -> !option.name().startsWith("TCP_"))
.collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for SOCK_STREAM 
here.


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other files, 
then the line 80 wouldn't become too long.


The same applies to src/java.base/unix/classes/java/net/PlainSocketImpl.java

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

Was it intentional that when flowSupported == true, quickAckSupported == 
false, keepAliveOptSupported == true, the TCP_KEEP* options are *not* 
added to the resulting set?


If not, then can this set population be organized in more linear way:  
Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman  wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very generic 
method method name. As I mentioned previously, you could simplify 
all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
static Set options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that we 
should deprecate-for-removal jdk/net/Sockets.java. It’s functionality 
is already supported by a standard API.







--
With kind regards,
Ivan Gerasimov

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

[vyom tewari]

thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman  wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...

It would be better if the channel implementation didn't static import 
ExtendedSocketOptions.getInstance as that is a very generic method method name. 
As I mentioned previously, you could simplify all these usages if you add the 
following to sun.net.ext.ExtendedSocketOption
static Set options(int type) { return 
getInstance().options(type)); }

+1


A minor comment on tests is that they can use List.of instead of Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that we should 
deprecate-for-removal jdk/net/Sockets.java. It’s functionality is already 
supported by a standard API.

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

[Alan Bateman]

On 11/05/2018 19:31, Chris Hegarty wrote:

:
P.S. A separate issue, but when reviewing this it reminded me that we should 
deprecate-for-removal jdk/net/Sockets.java. It’s functionality is already 
supported by a standard API.

I think just the methods rather than the class. Lucy Yingqi is working 
on RDMA support (discussion on the nio-dev list) that will add several 
new methods to jdk.net.Sockets.


-Alan.

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

[Chris Hegarty]

On 11 May 2018, at 01:04, Alan Bateman  wrote:
> 
> On 10/05/2018 16:21, vyom tewari wrote:
>> Hi, 
>> 
>> Please find the latest 
>> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
>> ...
> It would be better if the channel implementation didn't static import 
> ExtendedSocketOptions.getInstance as that is a very generic method method 
> name. As I mentioned previously, you could simplify all these usages if you 
> add the following to sun.net.ext.ExtendedSocketOption
>static Set options(int type) { return 
> getInstance().options(type)); }

+1

> A minor comment on tests is that they can use List.of instead of 
> Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that we should 
deprecate-for-removal jdk/net/Sockets.java. It’s functionality is already 
supported by a standard API.

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

[Alan Bateman]

On 10/05/2018 16:21, 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.

This looks much better.

As I also mentioned previously, it would be a lot cleaner to register by 
socket type rather than filter by socket option name but since it's now 
hidden in ExtendedSocketOptions then it's probably okay (and can be 
improved at a later time). For consistency, options(SOCK_STREAM) should 
filter "UDP_*" sockets and the method can throw IAE to reject unknown 
socket types.


It would be better if the channel implementation didn't static import 
ExtendedSocketOptions.getInstance as that is a very generic method 
method name. As I mentioned previously, you could simplify all these 
usages if you add the following to sun.net.ext.ExtendedSocketOption
   static Set options(int type) { return 
getInstance().options(type)); }


A minor comment on tests is that they can use List.of instead of 
Arrays.asList.


-Alan

Re: 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

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

[vyom tewari]

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.

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

[Langer, Christoph]

Ok, let's get some more opinions on that... 

> -Original Message-
> From: vyom tewari [mailto:vyom.tew...@oracle.com]
> Sent: Donnerstag, 26. April 2018 12:31
> To: Langer, Christoph <christoph.lan...@sap.com>; Chris Hegarty
> <chris.hega...@oracle.com>; OpenJDK Network Dev list  d...@openjdk.java.net>
> Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
> keepalive
> 
> 
> 
> On Thursday 26 April 2018 03:48 PM, Langer, Christoph wrote:
> > Hi Vyom,
> >
> > what about my suggestions for renaming?
> >
> > src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java ->
> src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
> > src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c ->
> src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
> till now we don't have file name like MacOSX* so i choose to leave
> as it is but if people think "MacOSXSocketOption" is more appropriate, i
> will change the filename name in my next webrev.
> 
> Thanks,
> Vyom
> >
> > This would be more consistent as we have:
> > src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> > src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
> > ...
> > src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
> > src/jdk.net/solaris/classes/jdk/net/SolarisSocketOptions.java
> >
> > Thanks
> > Christoph
> >
> >> -Original Message-
> >> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> >> vyom tewari
> >> Sent: Montag, 23. April 2018 14:06
> >> To: Chris Hegarty <chris.hega...@oracle.com>; OpenJDK Network Dev list
> >> <net-dev@openjdk.java.net>
> >> Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
> >> keepalive
> >>
> >> Hi,
> >>
> >> Please find the latest
> >>
> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.htm
> >> l).
> >> I incorporated  most of the review comments.
> >>
> >> Thanks,
> >>
> >> Vyom
> >>
> >>
> >> On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:
> >>> Vyom,
> >>>
> >>> On 13/04/18 10:50, vyom tewari wrote:
> >>>> Hi All,
> >>>>
> >>>> Please review the below code.
> >>>>
> >>>> BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298
> >>>>
> >>>> webrev :
> >>>> http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html
> >>> Here is some proposed wording for the JDK-specific extended socket
> >>> options specification.
> >>>
> >>> 1) Uses a consistent style across all three new options,
> >>>     and is in line with existing extended options.
> >>> 2) Renamed the options slightly, to improve readability
> >>>     ( they don't need to conform to the native option names )
> >>> 3) Reordered them so the build up is more logical
> >>> 4) Removed inheritance from server sockets
> >>> 5) Added standard verbiage about being "platform and
> >>>     system dependent
> >>> 6) Added typical values. I think this is useful, as it
> >>>     gives an idea to the developer, but maybe it could be
> >>>     misleading. Can be dropped if there are concerns.
> >>>
> >>> Can you please confirm that this is accurate, and also
> >>> will leave enough "wriggle" room when additional platform
> >>> support is added.
> >>>
> >>> ---
> >>>
> >>>      /**
> >>>   * Keep-Alive idle time.
> >>>   *
> >>>   *  When the {@link
> java.net.StandardSocketOptions#SO_KEEPALIVE
> >>>   * SO_KEEPALIVE} option is enabled, TCP probes a connection that
> >>> has been
> >>>   * idle for some amount of time. The default value for this idle
> >>> period is
> >>>   * system dependent, but is typically 2 hours. The {@code
> >>> TCP_KEEPIDLE}
> >>>   * option can be used to affect this value for a given socket.
> >>>   *
> >>>   *  The value of this socket option is an {@code Integer} that
> >>> is the
> >>>   * number of seconds of idle time before keep-alive initiates a
> >>> probe. The
> >>>   * socket option is specific to stream-orie

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

[vyom tewari]

On Thursday 26 April 2018 03:48 PM, Langer, Christoph wrote:

Hi Vyom,

what about my suggestions for renaming?

src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java -> 
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c -> 
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
till now we don't have file name like MacOSX* so i choose to leave 
as it is but if people think "MacOSXSocketOption" is more appropriate, i 
will change the filename name in my next webrev.


Thanks,
Vyom


This would be more consistent as we have:
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
...
src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
src/jdk.net/solaris/classes/jdk/net/SolarisSocketOptions.java

Thanks
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
vyom tewari
Sent: Montag, 23. April 2018 14:06
To: Chris Hegarty <chris.hega...@oracle.com>; OpenJDK Network Dev list
<net-dev@openjdk.java.net>
Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
keepalive

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.htm
l).
I incorporated  most of the review comments.

Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev :
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
    and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
    ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
    system dependent
6) Added typical values. I think this is useful, as it
    gives an idea to the developer, but maybe it could be
    misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

     /**
  * Keep-Alive idle time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. The default value for this idle
period is
  * system dependent, but is typically 2 hours. The {@code
TCP_KEEPIDLE}
  * option can be used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds of idle time before keep-alive initiates a
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  *  @since 11
  */
     public static final SocketOption TCP_KEEPIDLE
     = new ExtSocketOption("TCP_KEEPIDLE",
Integer.class);

     /**
  * Keep-Alive retransmission interval time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe after some amount
of time.
  * The default value for this retransmition interval is system
dependent,
  * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL}
option can be
  * used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds to wait before retransmitting a keep-alive
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  * @since 11
  */
     public static final SocketOption TCP_KEEPINTERVAL
     = new ExtSocketOption("TCP_KEEPINTERVAL",
Integer.class);

     /**
  * Keep-Alive retransmission maximum limit.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe a certain number of
times
  * before a connection is considered to be broken. The default
value for
  * this keep-alive probe r

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

[Langer, Christoph]

Hi Vyom,

what about my suggestions for renaming?

src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java -> 
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c -> 
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c

This would be more consistent as we have:
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
...
src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
src/jdk.net/solaris/classes/jdk/net/SolarisSocketOptions.java

Thanks
Christoph

> -Original Message-
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> vyom tewari
> Sent: Montag, 23. April 2018 14:06
> To: Chris Hegarty <chris.hega...@oracle.com>; OpenJDK Network Dev list
> <net-dev@openjdk.java.net>
> Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
> keepalive
> 
> Hi,
> 
> Please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.htm
> l).
> I incorporated  most of the review comments.
> 
> Thanks,
> 
> Vyom
> 
> 
> On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:
> > Vyom,
> >
> > On 13/04/18 10:50, vyom tewari wrote:
> >> Hi All,
> >>
> >> Please review the below code.
> >>
> >> BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298
> >>
> >> webrev :
> >> http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html
> >
> > Here is some proposed wording for the JDK-specific extended socket
> > options specification.
> >
> > 1) Uses a consistent style across all three new options,
> >    and is in line with existing extended options.
> > 2) Renamed the options slightly, to improve readability
> >    ( they don't need to conform to the native option names )
> > 3) Reordered them so the build up is more logical
> > 4) Removed inheritance from server sockets
> > 5) Added standard verbiage about being "platform and
> >    system dependent
> > 6) Added typical values. I think this is useful, as it
> >    gives an idea to the developer, but maybe it could be
> >    misleading. Can be dropped if there are concerns.
> >
> > Can you please confirm that this is accurate, and also
> > will leave enough "wriggle" room when additional platform
> > support is added.
> >
> > ---
> >
> >     /**
> >  * Keep-Alive idle time.
> >  *
> >  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
> >  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
> > has been
> >  * idle for some amount of time. The default value for this idle
> > period is
> >  * system dependent, but is typically 2 hours. The {@code
> > TCP_KEEPIDLE}
> >  * option can be used to affect this value for a given socket.
> >  *
> >  *  The value of this socket option is an {@code Integer} that
> > is the
> >  * number of seconds of idle time before keep-alive initiates a
> > probe. The
> >  * socket option is specific to stream-oriented sockets using the
> > TCP/IP
> >  * protocol. The exact semantics of this socket option are socket
> > type and
> >  * system dependent.
> >  *
> >  *  @since 11
> >  */
> >     public static final SocketOption TCP_KEEPIDLE
> >     = new ExtSocketOption("TCP_KEEPIDLE",
> > Integer.class);
> >
> >     /**
> >  * Keep-Alive retransmission interval time.
> >  *
> >  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
> >  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
> > has been
> >  * idle for some amount of time. If the remote system does not
> > respond to a
> >  * keep-alive probe, TCP retransmits the probe after some amount
> > of time.
> >  * The default value for this retransmition interval is system
> > dependent,
> >  * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL}
> > option can be
> >  * used to affect this value for a given socket.
> >  *
> >  *  The value of this socket option is an {@code Integer} that
> > is the
> >  * number of seconds to wait before retransmitting a keep-alive
> > probe. The
> >  * socket option is specific to stream-oriented sockets using the
> > TCP/IP
> >  * protocol. The exact semantics of this socket option are socket
> > type and
> >  * sys

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

[vyom tewari]

Hi Alan,

Thanks for review comments, please find my answers inline.

Vyom


On Monday 23 April 2018 06:04 PM, 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. The currently wording also 
begs questions as to whether the socket option means anything when 
SO_KEEPALIVE is not enabled. Also it probably should say something 
about whether it can be changed at any time or not.


The implementation looks much better but the filtering by socket 
option name is still a bit fragile. It might be better for 
ExtendedSocketOptions to mainain 3 sets of options, one for 
SOCK_UNSPEC, one for SOCK_STREAM, the other for SOCK_DGRAM. A map 
would work too. The register method can specify socket type and it's 
very obvious which sockets the option is intended to be used for.
I choose to overload the "options(short type)" over maintain multiple 
set of options because it was straight forward and  minimal changes were 
required. If i split the extended options to multiple set then it will 
increases the complexity i have to change the register method and i have 
to change "ifOptionSupported" as well. i feel it is not worth because we 
are not going to add socket options frequently.


All usages are now ExtendedSocketOptions.getInstance().options(...). 
Have you considered a static options(...) method to reduce the typing 
at each of the usage sites?

no, let me check if we can consider static options(..) method.


-Alan

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

[Chris Hegarty]

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.

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

[Alan Bateman]

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. The currently wording also begs 
questions as to whether the socket option means anything when 
SO_KEEPALIVE is not enabled. Also it probably should say something about 
whether it can be changed at any time or not.


The implementation looks much better but the filtering by socket option 
name is still a bit fragile. It might be better for 
ExtendedSocketOptions to mainain 3 sets of options, one for SOCK_UNSPEC, 
one for SOCK_STREAM, the other for SOCK_DGRAM. A map would work too. The 
register method can specify socket type and it's very obvious which 
sockets the option is intended to be used for.


All usages are now ExtendedSocketOptions.getInstance().options(...). 
Have you considered a static options(...) method to reduce the typing at 
each of the usage sites?


-Alan

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

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.


Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

    /**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is
 * system dependent, but is typically 2 hours. The {@code 
TCP_KEEPIDLE}

 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
    public static final SocketOption TCP_KEEPIDLE
    = new ExtSocketOption("TCP_KEEPIDLE", 
Integer.class);


    /**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount 
of time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} 
option can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPINTERVAL
    = new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


    /**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to 
affect

 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * maximum number of keep-alive probes to be sent. The socket 
option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPCOUNT
    = new ExtSocketOption("TCP_KEEPCOUNT", 
Integer.class);


-Chris.

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

[vyom tewari]

Hi Chris,


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

yes, below is perfect i will do the changes and send the updated webrev 
soon. Thanks for help, writing Java doc is always harder(4x) than 
writing code.

Vyom

---

    /**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is
 * system dependent, but is typically 2 hours. The {@code 
TCP_KEEPIDLE}

 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
    public static final SocketOption TCP_KEEPIDLE
    = new ExtSocketOption("TCP_KEEPIDLE", 
Integer.class);


    /**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount 
of time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} 
option can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPINTERVAL
    = new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


    /**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to 
affect

 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * maximum number of keep-alive probes to be sent. The socket 
option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPCOUNT
    = new ExtSocketOption("TCP_KEEPCOUNT", 
Integer.class);


-Chris.

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

[Chris Hegarty]

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId: https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

/**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is

 * system dependent, but is typically 2 hours. The {@code TCP_KEEPIDLE}
 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
public static final SocketOption TCP_KEEPIDLE
= new ExtSocketOption("TCP_KEEPIDLE", Integer.class);

/**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount of 
time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} option 
can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
public static final SocketOption TCP_KEEPINTERVAL
= new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


/**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to affect
 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the

 * maximum number of keep-alive probes to be sent. The socket option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
public static final SocketOption TCP_KEEPCOUNT
= new ExtSocketOption("TCP_KEEPCOUNT", Integer.class);

-Chris.

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

[Langer, Christoph]

Hi Vyom,

I had a quick glance through your changes.

Apart from the suggestions you've already got (use snprintf instead of string 
concatenation, method ordering...), I think 
"src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java" and " 
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c " should be renamed to 
MacOSXSocketOptions. to be consistent with the other platforms (Linux 
and Solaris at the moment).

Furthermore, I think most of the options can/should also be supported for AIX. 
I'm willing to contribute this. However, I should probably do this in a 
separate bug once you have completed your work on this one. I would also add 
TCP QUICKACK then.

Best regards
Christoph

> -Original Message-
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> vyom tewari
> Sent: Freitag, 13. April 2018 11:51
> To: OpenJDK Network Dev list 
> Subject: RFR:8194298 Add support for per Socket configuration of TCP
> keepalive
> 
> Hi All,
> 
> Please review the below code.
> 
> BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298
> 
> webrev :
> http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html
> 
> Currently Java supports SO_KEEPALIVE, whose default value is 7200
> seconds which is too long for most of the applications. This code change
> will allow us to set the keepalive
> parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will
> configure
> the idle time on per socket basis.
> 
> I did code changes for Linux & Mac only, support for other platforms can
> be added in future if needed.
> 
> Thanks,
> 
> Vyom

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

[vyom tewari]

On Sunday 15 April 2018 01:33 PM, Alan Bateman wrote:

On 14/04/2018 08:09, Alan Bateman wrote:

:

Can you update SocketChannel/SocketOptionTests.java to ensure that 
SocketChannel is test? We also need to ensure that the new options 
don't show up in the supportedOptions returned by the channels that 
don't support these new options.
Just on this point, I think this needs work in ExtendedSocketOptions 
so that the extended options are organized by socket type (STREAM or 
DGRAM). This will become a lot more obvious once you add tests for 
SocketChannel as its implementation will need a change to pick up the 
extended options for STREAM sockets. It will also avoid the filtering 
in PlainDatagramSocketImpl that you've added to work around the issue 
there.
Even i thought the same, when i added TCP_QUICKACK but somehow i choose 
not to do at that time. I will modify the tests  and will  do the other 
changes suggested by you.


Vyom


-Alan

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

[Alan Bateman]

On 14/04/2018 08:09, Alan Bateman wrote:

:

Can you update SocketChannel/SocketOptionTests.java to ensure that 
SocketChannel is test? We also need to ensure that the new options 
don't show up in the supportedOptions returned by the channels that 
don't support these new options.
Just on this point, I think this needs work in ExtendedSocketOptions so 
that the extended options are organized by socket type (STREAM or 
DGRAM). This will become a lot more obvious once you add tests for 
SocketChannel as its implementation will need a change to pick up the 
extended options for STREAM sockets. It will also avoid the filtering in 
PlainDatagramSocketImpl that you've added to work around the issue there.


-Alan

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

[Alan Bateman]

On 13/04/2018 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Currently Java supports SO_KEEPALIVE, whose default value is 7200 
seconds which is too long for most of the applications. This code 
change will allow us to set the keepalive 
parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will 
configure the idle time on per socket basis.


I did code changes for Linux & Mac only, support for other platforms 
can be added in future if needed.
Limiting this to specific platforms is okay but I think their 
descriptions will need a bit to work to ensure there is enough wriggle 
room to support varying behavior and also be somewhat consistent with 
the wording that we use for other options. There are several points that 
will need to expanded in the javadoc and maybe the style/wording in 
java.net.StandardSocketOption#SO_KEEPALIVE would help get that somewhat 
consistent -- e.g. initial value, can be set on unbound socket, can be 
changed after being set, etc.


One concern is that you've chosen to support it on ServerSocket and 
specify that it can be "inherited" by the sockets for accepted 
connections. This isn't tested in the proposed tests and I'm wondering 
if we would be saner to limit this to connected sockets.


Can you update SocketChannel/SocketOptionTests.java to ensure that 
SocketChannel is test? We also need to ensure that the new options don't 
show up in the supportedOptions returned by the channels that don't 
support these new options.


In passing:  The ordering of the modifiers in XXXSocketOptions looks a 
bit odd, can this be changed to the more usual "private static native".


-Alan

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

[vyom tewari]

On Saturday 14 April 2018 01:40 AM, Bernd Eckenfels wrote:

Hello,

Glad to see progress on this, much needed.

thanks


I wonder if there is a better way for safe and dynamic string 
concatenation in the JDK, this errmsg[56] looks scary.

sure, i will fix it.


Did you tried to support it on Windows, even if it does not support 
all 3 parameters it might be important to be available (I think Oracle 
Database supports it for DCD on Windows, too)
I already mentioned in mail, support for other platform can be added in 
future if needed. Only "Windows 10 1709" support all three socket 
options(TCP_KEEPIDLE, TCP_KEEPCNT, TCP_KEEPINTVL).  These are socket 
options, we want set/get both, I tried to implement  keepalive times 
using WSAloctl 
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd877220(v=vs.85).aspx) 
but  it is not possible to get the current *SIO_KEEPALIVE_VALS* for a 
socket, MSDN says that WSAIoctl output buffer is not used.


Thanks,
Vyom


Gruss
Bernd

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

*From:* net-dev  on behalf of vyom 
tewari 

*Sent:* Friday, April 13, 2018 11:50:39 AM
*To:* OpenJDK Network Dev list
*Subject:* RFR:8194298 Add support for per Socket configuration of TCP 
keepalive

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html 



Currently Java supports SO_KEEPALIVE, whose default value is 7200
seconds which is too long for most of the applications. This code change
will allow us to set the keepalive
parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will configure
the idle time on per socket basis.

I did code changes for Linux & Mac only, support for other platforms can
be added in future if needed.

Thanks,

Vyom

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

[Bernd Eckenfels]

Hello,

Glad to see progress on this, much needed.

I wonder if there is a better way for safe and dynamic string concatenation in 
the JDK, this errmsg[56] looks scary.

Did you tried to support it on Windows, even if it does not support all 3 
parameters it might be important to be available (I think Oracle Database 
supports it for DCD on Windows, too)

Gruss
Bernd

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

From: net-dev  on behalf of vyom tewari 

Sent: Friday, April 13, 2018 11:50:39 AM
To: OpenJDK Network Dev list
Subject: RFR:8194298 Add support for per Socket configuration of TCP keepalive

Hi All,

Please review the below code.

BugId: https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Currently Java supports SO_KEEPALIVE, whose default value is 7200
seconds which is too long for most of the applications. This code change
will allow us to set the keepalive
parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will configure
the idle time on per socket basis.

I did code changes for Linux & Mac only, support for other platforms can
be added in future if needed.

Thanks,

Vyom