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

[Alan Bateman]

On 08/05/2020 18:23, Michael McMahon wrote:
> :
>
> It's not just Windows and it's hard to discern patterns from the test, 
> the way it is structured,
> but another one would be binding the first socket to an IPv4 address 
> (loopback or actual local address)
> and binding the second socket to an IPv6 equivalent. This fails for 
> SocketChannel on all platforms,
> and also for DatagramChannel on all platforms except MacOS.
>
> If you want I can cut the test back to only test positive scenarios 
> that should work across all platforms.
Negative scenarios are good too but I think we have to weed out the test 
cases that don't make sense.

I think the main issue with the matrix is that very row with saddr=null 
or saddr=xxANYLOCAL creates a nonsensical scenario for 
DatagramChannel.connect. Maybe you could put a special value "should 
pass on DC" element for these rows so that they are skipped?

The other set of issues arises with the rows that have a value other 
than ALL or ~ALL in the "should pass on SC" column. The ML elements seem 
to be cases on Windows where a listener is bound to the wildcard address 
but the client cannot connect using the loopback address, is that right? 
I think we need a bit more information on what is going on there as I 
would expect that should work everywhere. (A subtle change in the patch 
is that it switch connect-to-wildcard to connect to the loopback rather 
than host address, but I don't think this explains it).

There are a few other fishy. Is it possible that IP4LOCAL and IP6LOCAL 
are non loopback addresses on some test systems?  There are several left 
over comments such as "hangs L", and "true on macOS??" and I wonder if 
this might be the root of some of platform variance that you are running 
into.

The rename/move of LocalSocketAddressType is good. You might want to 
double-check the addresses data provider as it's not clear (to me) why 
it adds InetAddress.getLocalHost and getLoopbackAddress to the stream, 
don't you end up with duplicates? I would expect you could siumplify it 
down to something like:

         NetworkConfiguration nc = NetworkConfiguration.probe();
         return Stream.concat(nc.ip4Addresses(), nc.ip6Addresses())
                 .map(ia -> new Object[] { new InetSocketAddress(ia, 0) })
                 .iterator();

A minor is is that the "throws Exception" in each of the tests have 
ended up on their own line, no need for that I assume.

-Alan.