RFR 8224477 [13] java.net.Socket::setOption - implementation mismatch to spec

[Alan Bateman]

On 22/05/2019 13:54, Chris Hegarty wrote:
>
> While we're here, let's address both the NPE and the IAE.
Okay, I only mentioned it because the JIRA issue had two cases. Also 
it's one that we came across when working on the new implementation (it 
is called out as a compatibility difference in the Risks and Assumptions 
second). Part of this is that Socket.setOption specifies IAE for invalid 
values but SocketImpl.setOption doesn't specify anything. The validation 
of the value has to be done by the SocketImpl as it may relate to socket 
options that the Socket API doesn't know anything about so I think 
SocketImpl.setOption has to specify IAE. I'm not suggesting we expand 
the scope of the issue here but they are related.

>
> Updated webrev:
> https://cr.openjdk.java.net/~chegar/8224477/webrev.01/index.html
>
> The code to check that specific option values are within the specified
> range is independent of the particular socket type. I decided to add it
> to StandardSocketOptions for two reasons: 1) co-location with its
> textual range specification, and 2) is it currently only required by the
> plain socket implementations. This could be located elsewhere, and
> possibly shared by other code performing range checking ( since these
> checks need to be consistent ).
>
> Since the test is more involved now, I created and new separate test, so
> there will be no conflict with upcoming changes to OptionsTest.
I went through the webrev.

The new test looks great. A suggestion for the name is NullsAndBadValues 
as that sort of sums up what it does.

One comment on AbstractPlainSocketImpl.setOption is that it means calls 
to the impl setOption will call PlainSocketImpl.setOption, it will 
delegate to the super class, which will in turn delegate to its super 
class. I'm just wondering if the PlainSocketImpl.setOption can be 
subsumed into AbstractPlainSocketImpl.

I don't think StandardSocketOptions is the right place for 
throwIfIllegalValue. It may be convenient but this class defines 
constants and wasn't really meant to be a helper class too. I guess one 
could make StdSocketOption package private and add a validate check but 
I think that would complicate it too. The check also reminds me that 0 
is valid as the send/receive buffer size in some APIs but not in others. 
This may be something we re-visit in time as it might make sense to have 
IAE throw on platforms where 0 is not valid.

>
> I verified the test against the NioSocketImpl, and it passes fine for
> all cases except one, SO_REUSEADDR. The NioSocketImpl::setOption method
> throws NPE instead of IAE when passed a null value. Easy to resolve,
> then both socket implementations behave consistently, with regard to
> exceptions being thrown for bad input.
Thanks for checking this. Windows uses exclude bind so SO_REUSEADDR is a 
no-op, that code path needs to check the value is null before casting it 
to a Boolean.

>
> Once we agree the code changes, I'll file a CSR to track the change in
> behaviour.
Right, there's behavior change so a CSR make sense.

-Alan