RFR 8241305: Add protocol specific factory creation methods to SocketChannel and ServerSocketChannel

[Alan Bateman]

On 05/05/2020 12:16, Michael McMahon wrote:
> Hi,
>
> Could I get the following change reviewed please?
>
> The change is to add new static factory creation methods
> to java.nio.channels.SocketChannel and 
> java.nio.channels.ServerSocketChannel
> which take a java.net.ProtocolFamily. These are basically the same as
> a similar method that was added to DatagramChannel back in 1.7
> and which allow the creation of IPv4 only channels or IPv4/IPv6.
The javadoc and implementation looks good.

Minor comment is that the requiresNonNull is not really needed in 
SocketChannel, ServerSocketChannel and DatagramSocket as they just 
delegate to the provider which is specified to throw NPE already. Also 
the second/subsequent lines of the @implSpec in SelectorProvider isn't 
misaligned. It might be simpler to just not indent those lines.

The convention in this area is to organize the tests by class name. 
There is an etc directory for tests that cover many channels that might 
work.

OpenDefault.java - the @summary says that it tests the "no-arg open 
method" but the test actually tests the socket address type returned by 
the adaptor's getLocalAddress and getInetAddress methods. It read is as 
adding test coverage to the existing APIs rather than the new APIs. A 
few comments:
- I think we need to find a better name for this test, maybe 
LocalSocketAddressType ?
- Can it be expanded to test getLocalAddress rather than just the socket 
adaptor methods?
- The test names are openXXX but might be clearer if renamed to 
something like testSocketChannel, testServerSocketChannel, .. as it 
tests the local addresses that these channel report.

OpenConnect.java - this is a good combo test and you might want to beef 
up the @summary to more fully describe what it does. You might want to 
think about renaming too, maybe OpenAndConnect. A few questions/comments:
- Would it be possible to summarize the platform specific issues that 
you are running into? Most of the MLS elements seem to be 
DatagramChannel and it would be useful to get a summary of that issue (I 
suspect part of it is trying to connect a UDP connect to the wildcard 
address, something that doesn't make sense).
- Why is this test /othervm and is /timeout=30 needed?
- What is the purpose of cfactory and sfactory? I can't see how they 
could ever be null. If they are removed then it would allow the static 
openXXX methods to be cleaned up. At the use-sites it checks for null 
and I assume they should go away too.
- I think linkLocalAddr should be removed or renamed to something like 
ipv6LinkLocalAddress to make it clearer that it returns true when the 
address is an IPv6 link local address. Alternatively might be clearer to 
just filter at the two use-sites.
- connectNonBlocking doesn't look quite right. The connect method 
returns a boolean to indicate if the connection is established. If it 
returns false then you can register it with the Selector and select.
- A minor point is that the parameters can be ProtocolFamily rather than 
StandardProtocolFamily.
- There are a couple of places the exceptions have unusual names, e.g. 
catch (UnsupportedOperationException re), catch (Exception re).  Where 
these RuntimeException at some stage, maybe they can be renamed now.

I skimmed through the ProtocolFamilies test too and have several 
comments but don't have time today to write all those down, I'll get to 
in the next few days.

-Alan.