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

[Chris Hegarty]

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/

-Chris.


> On 23 May 2019, at 09:13, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> 
> 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