On 04/05/2020 22:21, Ivanov, Vladimir A wrote:
Thanks for your comments.
Updated version of the patch available as
http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.07/
The exception part for the setOption method was updated
+ } else if (option == SO_INCOMING_NAPI_ID) {
+ if (!incomingNapiIdOptSupported)
+ throw new UnsupportedOperationException("Attempt to set
unsupported option " + option);
+ else
+ throw new IOException("Attempt to set read only option
" + option);
It required to update the method's description. As side effect some application
may report compile error for the 'setOption' method usage while one more
exception should be added to processing.
The Javadoc and the ExtOptionNAPITest.java were updated to use IOE instead of
UOE.
Tests "test/jdk/jdk/net/Sockets test/jdk/java/net/SocketOption/AfterClose.java
test/jdk/java/nio/channels/etc/PrintSupportedOptions.java" passed on the
RHEL8 (with NAPI support) and the RHEL7.7 (without NAPI support).
A SocketException is an IOException so no need for both in the setOption
declaration. The method is internal so it's not going to impact anyone
using any of the supported APIs. That said, it might be more consistent
to throw SocketException here and that will avoid needing to change the
declarations.
So assuming s/IOException/SocketException/ then I think the javadoc
looks okay. A residual question from the discussion is whether you want
to make it clear that the identifier is a positive Integer. Once we have
the javadoc agreed here then you should get the CSR submitted.
I don't think ExtOptionNAPITest is ready for review. I think we need a
simpler test that is reliable and easy to maintain. It will need to
exercise each of the network channels (defined in java.nio.channels) in
addition to the socket APIs defined in java.net. It is important that
the listener binds to an ephemeral port as we can't used fixed ports in
tests. It's also important that all tests do cleanup (the current
ExtOptionNAPITest leaves many sockets open). We can iterate on the test
here in parallel with the CSR.
-Alan
