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

[Michael McMahon]

On Mon, 5 Oct 2020 15:09:53 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

>> 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.

> _Mailing list message from [Alan Bateman](mailto:Alan.Bateman at oracle.com) on
> [nio-dev](mailto:nio-dev at openjdk.java.net):_
> I went through the latest patch, v18.
> 
> I'm still a bit uncomfortable with splitting up SocketChannelImpl and
> ServerSocketChannelImpl but okay with go along with this for now if we
> can continue explore alternatives. Given that the InetXXX and UnixXXX
> sub-classes aren't too big then one thing that we can try is just
> special casing the protocol family in the 4-5 methods that are
> different. In the mean-time, I think the abstract methods in
> SocketChannelImpl and ServerSocketChannelImpl should have a clear method
> description on what the implXXX method should. I would prefer the other
> methods be changed to final so that it's clear that they cannot be
> overridden. implSocket is new in v17 or v18 and I'd prefer if that were
> abstract so it can be implemented in UnixXXX to throw UOE.
> 

Okay, I'll take a look at pulling all the changes back into one SocketChannelImpl
and ServerSocketChannelImpl.

> I think the JFR events need another look. Are UnixSocketReadEvent and
> UnixSocketWriteEvent really needed? SocketXXXEvent "address" is just a
> String so it can be used for any address type, the port can be left as
> 0. Part of this comment is wondering if a parallel set of events are
> really needed. Part of it that I would prefer if the read/write methods
> were final in SocketChannelImpl.
> 

The existing socket read/write events include these fields:

    @Label("Remote Host")
    public String host;

    @Label("Remote Address")
    public String address;

    @Label("Remote Port")
    public int port;

I guess, address could be used to encode a path name. port could just be set to -1
and host could be hard coded to some string which indicates the event relates to
a Unix domain socket. I was originally thinking "localhost" would be a good value,
but it would need to be something more explicit than that, to clearly indicate the
socket type, because remote addresses will often be empty path names.

It could be a string like "Unix domain socket" or if a string with white-space embedded
might cause a problem, then something like  AF_UNIX:<path\> where path is the address (which
might be empty)? Which wouldn't look great imo. I guess some guidance from JFR experts
would help here ...

All comments below look fine.

Thanks
Michael

> You might want to take a pass over the files in the patch as several of
> the new files have copyright dates starting 2012, 2019 and other years,
> probably initially copied from other files.
> 
> A few specific comments on v18:
> 
> src/java.base/share/classes/sun/nio/ch/UnixDomainSocketChannelImpl.java
> - supportedOptions doesn't need to wrap the unmodifable collection with
> Collections.unmodifiableSet
> - I think implBind should be re-worked so the local != null case is not
> handled in the loop body. That will help future maintainers.
> 
> src/java.base/windows/classes/sun/nio/ch/UnixDomainSocketsUtil.java
> - I think we should look at adding JAVA_IO_TMPDIR to StaticProperty.
> 
> src/java.base/unix/native/libnio/ch/UnixDomainSockets.c
> - are the AIX specific includes really needed?
> - can unixSocketAddressToSockaddr use if-then-else to avoid the goto.
> - bind0 checks if the path is NULL, is this a left over?
> - the declaration of accept0 is mis-aligned, might be left over from
> refactoring
> 
> src/java.base/unix/native/libnio/ch/InheritedChannel.c
> - might be clearer if matchFamily is renamed to toInetFamily
> 
> src/java.base/unix/classes/sun/nio/ch/InheritedChannel.java
> - peerAddress0 is replaced by peerAddressInet and peerAddressUnix0 which
> looks a bit consistent, maybe rename those to inetPeerAddress0 and
> unixPeerAddress0 ?
> 
> src/java.base/windows/classes/sun/nio/ch/PipeImpl.java
> - we've gone through a few iterations on this and only a few minor comments
> - source/sink can be final and I assume no need to change their type
> from SourceChannel/SinkChannel
> - no need to initialize noUnixDomainSockets to false
> - can the comments at L178-179 be turned into a method description.
> 
> Can the tests for java.net.UnixDomainSocketAddress be moved to
> jdk/test/java/net/UnixSocketAddress?
> 
> Could test/jdk/java/nio/file/spi/TestProvider.java be used as
> alternative provider in the test to avoid DummyPath?
> 
> test/jdk/java/nio/channels/etc/ProtocolFamilies.java has been updated to
> add "expectedFamily", is that used?
> 
> test/jdk/java/nio/channels/unixdomain/WindowsDriver.java and
> NonWindowsDriver.java
> - are these needed now? Both of them run the same 5 tests and not clear
> why these tests don't have their own @test tags.
> 
> test/jdk/java/nio/channels/unixdomain/Security.java
> - is there any reason why this test can't run with run
> main/othervm/java.security.policy=java.policy1 -Djava.security.manager ?
> 
> That's all I have from this pass over the changes.
> 
> -Alan

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

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