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

[Alan Bateman]

On 08/05/2020 10:41, Michael McMahon wrote:
> :
>
> If you have any comments about the third test, let me know when you 
> get a chance
>
I found a bit of time to look at the 3rd test, ProtocolFamilies. It 
provides good coverage but will need a round or two of cleanup.

What is the purpose of running with -Djava.net.preferIPv6Addresses=true? 
That influences the ordering of lookups with InetAddress and I can't 
obviously see how it relates to this test.

ia4 = getFirstLinkLocalIPv4Address().  The naming is problematic as it 
doesn't return a link-local address (it might do on a mis-configured 
system but we usually try to avoid such addresses). Instead it finds a 
local IPv4 address that is not a loopback address.  Maybe rename it 
findLocalIPv4Address and put a comment that it may return null?

ia6 = getFirstLinkLocalIPv6Address().  Do you really want an IPv6 link 
local address or just a local address that is not the loopback? (I'm 
asking as its different to the IPv4 version on a number of points, 
including returning ::0 rather than null when an address is not found).

Is dcOpenConnect needed in this test? This has the same problem as the 
other test where it is trying to connect a UDP socket to the wildcard 
address so doesn't make sense.

It looks like the xxOpen tests are leaking file descriptors. Is this the 
reason why the default @run is /othervm? I'd prefer if the sockets were 
closed so that we at least have the option of running the test in agent 
VM mode. A minor comment is that I think I'd prefer if the parameters 
the xxOpen methods were "family" and something like "expectedException".

The exception expected in the xxOpenBind tests with the openBind data 
provider is a bit confusing. It looks like the rows with "UOE" for the 
IPv4-only case are dups of the xxOpen tests? If you drop these rows then 
it would avoid special casing UOE and would allow you to drop "open" 
from the names too.

defaultImpl() seems to test a custom SelectorProvider (not the default 
implementation) so maybe it should be renamed to testCustomProvider and 
SELECTOR_PROVIDER should be renamed too.

Trivially, "nulls" and "uoe" might be better as testNulls and testUOE. 
Naming the unknown protocol family "STITCH" looks a bit strange, can 
this be something more obvious such as "CUSTOM_PF", "UNKNOWN_PF" or 
"BAD_PF"?

-Alan.