On 23/05/2019 17:32, Chris Hegarty wrote:
Thank you Alan,
I believe that I addressed all your comments and suggestions.
Additionally, while here I noticed an issue where these methods were not
always consistent with their spec to throw IOException when the socket
has been closed.
https://cr.openjdk.java.net/~chegar/8224477/webrev.02/
The isClosed checks look fine although a Socket/SS/DS could be
asynchronously closed so needs to be handled by the impl when the Socket
doesn't provide the coordination with close. This is a long standing
rats nest of course, and should go away with the new implementation.
The new AfterClose and the now renamed NullsAndBadValues seems very
comprehensive.
I have a few comments on the new setOption/getOption implementation:
- The value == null check isn't needed as it will be caught by the
isInstance(value) check
- I think the checks in StdSocketOptionUtil.throwIfBadValue should be
moved into SocketImpl.setOption and DatagramSocketImpl.setOption. That
is, these methods map new to old and I think are the right place to
reject values that should throw IAE. This should avoid the override need
to validate the values before delegating to the super class.
- I think SocketImpl.setOption needs the same
name.type().isInstance(value) check. Not important but would ensure that
any pre JDK 9 SocketImpls throw the right exception (the issue of
SocketImpl.setOption not specifying IAE is another issue of course).
-Alan
