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

[Alan Bateman]

On 04/10/2020 14:14, Michael McMahon 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
>
> -------------
>
> Changes:
>    - all: https://git.openjdk.java.net/jdk/pull/52/files
>    - new: https://git.openjdk.java.net/jdk/pull/52/files/dc70cd6e..0096645e
>
> Webrevs:
>   - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=13
>   - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=12-13
>
>    Stats: 9 lines in 4 files changed: 0 ins; 4 del; 5 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/52.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52
>
> PR: https://git.openjdk.java.net/jdk/pull/52
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.

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?

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.

src/java.base/share/classes/sun/nio/ch/UnixDomainServerSocketChannelImpl.java
- implBind should not loop/retry when a local address is provided
- what behavior is expected when getTempDir returns null?
- Why doesn't this code use SecureRandom, as is done in the Windows 
Selector implementation.
- Can supportedOptions be changed to return 
Set.of(StandardSocketOptions.SO_RCVBUF)?
- 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.
- 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.

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

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.

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.