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

[Michael McMahon]

I have updated the webrev for this at 
http://cr.openjdk.java.net/~michaelm/8241305/webrev.4/
based on the last round of comments.

Thanks,

Michael.

On 09/05/2020 15:25, Alan Bateman wrote:
> 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.