On Mon, 5 Oct 2020 12:58:52 GMT, Erik Joelsson <er...@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.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. ------------- PR: https://git.openjdk.java.net/jdk/pull/52
