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.bate...@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
