RFR: 8245194: Unix domain socket channel implementation [v14]

[Michael McMahon]

On Mon, 5 Oct 2020 12:58:52 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Michael McMahon has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   - simplified Copy.gmk to CAT source files directly
>>   - renamed net.properties source files to all be net.properties
>
> Build changes look good.

Thanks again Alan. Assume where there is no comment from me below that suggestions are accepted:
I will push an update based on these changes soon.

Michael.

> _Mailing list message from [Alan Bateman](mailto:Alan.Bateman at oracle.com) on
> [nio-dev](mailto:nio-dev at openjdk.java.net):_
> On 04/10/2020 14:14, Michael McMahon wrote:
> Another round of comments, this time on v14.
> 
> src/java.base/share/classes/java/net/StandardProtocolFamily.java
> - the enum constant is "UNIX" and might be better to put "Local" in
> parentheses rather than "Unix domain".
> 
> src/java.base/share/classes/java/nio/channels/package-info.java
> - L260 ends with "family only", I think "only" can be dropped.
> 
> src/java.base/share/classes/java/net/UnixDomainSocketAddress.java
> - left over "fix message" comments in constructor
> - a few formatting nits in the equals method due to a spurious space in
> the expression at L192, and missing a space at L194.
> 

I don't see a missing space at L194 ?

> src/java.base/share/classes/sun/net/util/SocketExceptions.java
> - Can of(IOException e, SocketAddress addr) check enhancedExceptionText
> to avoid the ugly check in the ofXXX methods?
> - Can you explain the inconsistency in the null handling for the address
> types, maybe a left over from an early prototype?
> 

The null check was always there for Inet sockets. It is not required for Unix
but would be benign. So, the check can be promoted to the calling method.

> src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java
> src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java
> - I think we need to change the class descriptions of both to start with
> "Base implementations ..."
> - We need to find a clean way to push the socket() method down to the
> InetXXXImpl classes as they are the only classes that should know about
> the socket adaptors.

Only way to do this is to define yet another implXXX method (implSocket)
but I think it is worthwhile as neither the field nor the implementation
belong in the super class

> 
> src/java.base/share/classes/sun/nio/ch/UnixDomainServerSocketChannelImpl.java
> - implBind should not loop/retry when a local address is provided

It actually catches the BindException and rethrows it when 'local != null, but maybe
the comment above the loop is confusing. I will reword it.

> - what behavior is expected when getTempDir returns null?

I was assuming that ${java.io.tmpdir} would always be defined, but I see
getTempDir() could be more robust in dealing with errors in the preceding
search steps. So, in the unlikely event that ${java.io.tmpdir} is not defined
null would be returned. I will change to throw a BindException in that case.

> - Why doesn't this code use SecureRandom, as is done in the Windows
> Selector implementation.

It does use SecureRandom but only uses the Random api.

> - Can supportedOptions be changed to return
> Set.of(StandardSocketOptions.SO_RCVBUF)?

I can simplify that code definitely, but I think for consistency it should
still return a static unmodifiable instance

> - toString has a left over "TODO".
> - The message for the IOException in implBind should start with a
> capital to be consistent with the other exceptions thrown in this area
> 
> src/java.base/share/classes/sun/nio/ch/UnixDomainSockets.java
> - The initializer sets the system property
> "jdk.nio.channels.unixdomain.maxnamelength". Looks like it's to help the
> tests so need to find another solution for the tests.

Okay, the tests didn't really need this anyway

> - checkCapability should be renamed to checkPermission as it does a
> security manager check. Would it more be consistent with existing code
> if changed to "if (sm != null) sm.checkPermission(...)"
> - Can inTempDirectory be removed as it doesn't seem to be used and
> raises several questions if it is needed.
> - getTempName should use the path separator rather than "/".? Also I
> don't think "nio" should be in the name.
> - Can the init method be removed, may be a left over from a previous
> iteration.
> - The left breaks in the initialization of UNNAMED and support seem
> unnecessary, maybe they were very long in previous iterations?
> - Several methods are public, is that intentional, or maybe a left over?
> 
> src/java.base/unix/native/libnio/ch/UnixDomainSockets.c
> => Is the intention to put the NET_ functions into libnet or libnio? The
> function prototypes are declared in net_util.h but the functions have
> been compiled in libnio.
> 

They were probably in libnet originally, but makes more sense to be
in libnio. So, I will move the function prototypes to nio_util.h and rename
them to remove the NET_ prefix.

> src/java.base/share/classes/sun/nio/ch/Net.java
> - spurious blank line, probably left over from a previous iteration.
> 
> src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java
> src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
> - getSunPathForSocketFile should not accept null, let
> to{Unix,Windows}Path handle it
> 
> src/java.base/windows/classes/sun/nio/ch/PipeImpl.java
> src/java.base/windows/classes/sun/nio/ch/SinkChannelImpl.java
> src/java.base/windows/classes/sun/nio/ch/WindowsSelectorImpl.java
> - The changes to PipeImpl look like they will set TCP_NODELAY for users
> of the Pipe API on older Windows releases. I think I would prefer to see
> a parameter on the PipeImpl constructor to indicate if there should
> buffered/nodelay or not. That way it will be clearer for the two usages
> of PipeImpl.

Right, good point.

> - The volatile noUnixDomainSockets does not need to be initialized to false.
> 
> src/jdk.net/share/classes/jdk/net/UnixDomainPrincipal.java
> - if records are finalized in Java 16 then this may be a candidate to be
> a record
> 

Right. I don't think there is anything we can do further we can do right now
on this point, but if records make it, then it would be a simple API change to make.

> test/jdk/java/nio/channels/etc/ProtocolFamilies.java
> - minor nit but the existing test uses "ssc", "sc", and "dc" so best to
> keep it consistent if you can.
> 
> test/jdk/jdk/nio/Basic.java
> - can you change this to use
> Class.forName("sun.nio.ch.InetSocketChannelImpl"), that should be
> cleaner and void the retry and introducing getFD1.
> 

Good idea, but needs to be "sun.nio.ch.SocketChannelImpl" I think

> I'm still working through the changes to the InheritedChannel
> implementation and all the changes/additions of tests so more comments
> soon on that aspect of the patch.
> 
> -Alan.

-------------

PR: https://git.openjdk.java.net/jdk/pull/52