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

Michael McMahon michaelm at openjdk.java.net
Tue Oct 13 17:36:12 UTC 2020 

On Mon, 12 Oct 2020 11:02:12 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

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

So, I have done the refactoring and other work suggested and I think it was worthwhile. We're back to only two impl
classes now (from six). The JFR changes are simplified (at the cost of shoe-horning Unix domain addresses into the Inet
structure) but it's probably beneficial to have only one read and write event type for all sockets. When I went through
the comments below, there was one thing I missed in my reply yesterday. See below:

I will push these changes to the branch shortly.

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

I added a package private method so SinkChannelImpl for accessing the channel. So, sink needs to be a SinkChannelImpl
at least. Also, the fields are not assigned in the constructor. So, don't think they can easily be made final.

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

More information about the hotspot-jfr-dev mailing list