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
