On Mon, 12 Oct 2020 11:02:12 GMT, Michael McMahon <micha...@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.bate...@oracle.com) on >>> [nio-dev](mailto:nio-...@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.bate...@oracle.com) on >> [nio-dev](mailto:nio-...@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.bate...@oracle.com) on > [nio-dev](mailto:nio-...@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
