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

[Michael McMahon]

Hi Alan

I think we have dealt with your comments below. As regards the question 
about platform
specific differences shown by the OpenAndConnect test, yes much of it 
results in the case
where the server binds to a wildcard address and then the client 
connects to that.
It was a surprise to me that the API supports that at all, which it 
does. So, the test
is really just documenting the differences. It does seem to be mostly 
windows that disallows it.

Also, I didn't remove the null checks for protocol family. While it is a 
duplication with respect
to the default provider, it does cover the case where a provider does 
not perform the check.
I don't have a strong feeling about it, and don't mind removing it.

I have updated the webrev at: 
http://cr.openjdk.java.net/~michaelm/8241305/webrev.2/

If you have any comments about the third test, let me know when you get 
a chance

Thanks,

Michael

On 05/05/2020 13:53, Alan Bateman wrote:
> 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.
>
>