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

[Alan Bateman]

On 24/09/2020 13:17, Michael McMahon wrote:
> Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since
> the last revision:
>
>   - Merge branch 'master' into unixdomainchannels
>   - unixdomainchannels: some tests were failing on Windows version prior to 10/2019
>   - unixdomainchannels: updates after comments sent by Alan 14 Sept
>   - unixdomainchannels: updates from review of Sept 11 2020
>   - Merge branch 'master' into unixdomainchannels
>   - Made some changes relating to selection of the local directory where automatically
>     bound server sockets are created. After this change it is no longer necessary
>     to specify the location in individual tests.
>   - unixdomainchannels: fixing compile error on Windows and Alan's review comment this morning
>   - unixdomainchannels: initial commit from hg sandbox with changes from Alan's email 06/09/2020
>
> -------------
>
> Changes:
>    - all: https://git.openjdk.java.net/jdk/pull/52/files
>    - new: https://git.openjdk.java.net/jdk/pull/52/files/d29e8cc2..a08186d7
>
> Webrevs:
>   - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=06
>   - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=05-06
>   
I've pulled the latest commits from jdk/pull/55 and have another set of 
comments.

src/java.base/share/classes/sun/nio/ch/InetSocketChannelImpl.java
src/java.base/share/classes/sun/nio/ch/InetServerSocketChannelImpl.java
- class level comment is out dated after the split so should be updated 
to make it clear that it for Inet implementations.
- the comment "End of field protected by stateLock" has been copied down 
from ServerSocketChannelImpl.java and is confusing here so I think 
should be removed.
- The existing convention is to declare the final fields at the top so 
best to keep consistent after the split.

src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
- the change to isStreamOption would be cleaned if the "SO_FLOW_SLA" 
case is removed (I think it was missed by JDK-8244582).

src/java.base/share/classes/sun/net/util/SocketExceptions.java
- it's a bit inconsistent for of(IOException, SocketAddress) to handle 
InetSocketAddress types itself, it would be cleaner to handle both 
address types or use two helper methods.
- Is the null check in ofUnixDomain left over from a previous iteration?

src/java.base/share/classes/sun/nio/ch/SelectorProviderImpl.java
- it would be help to split the really long lines to keep it consistent 
with the existing code

src/java.base/share/classes/sun/nio/fs/AbstractFileSystemProvider.java
- it might be cleaner to rename the method to getSunPathForSocketFile. 
It doesn't access the file so would be better not to throw IOException.
- for the description it might be clearer to say that it "Returns a path 
name as bytes for ..."

src/java.base/share/native/libnet/net_util.h
- are you sure that struct sockaddr_un is defined on all platforms, 
including slightly older VC++ releases.
- the lines are a bit long here, inconsistent with the existing code.

src/java.base/unix/classes/sun/nio/ch/UnixDomainHelper.java
- I think this should be removed and the method included in 
UnixDomainSockets
- The hardcoding of /tmp and /var/tmp needs to be dropped too. We also 
need to find a name name for jdk.nio.channels.tmpdir as this is specific 
to Unix domain sockets.
- you can avoid the ugly cast by creating a PrivilegedAction and then 
return doPrivileged(pa).

src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java
- if you rename the parameter to obj and use UnixPath file = 
UnixPath.toUnixPath(obj) then it will be consistent with the existing 
code. It will also ensure that ProviderMismatchException is thrown.

src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
- this can use WindowsPath file = WindowsPath.toWindowsPathobj)

src/java.base/windows/classes/sun/nio/ch/PipeImpl.java
src/java.base/windows/classes/sun/nio/ch/SinkChannelImpl.java
- I think the change to SinkChannelImpl.java should be dropped. Instead, 
move the setting TCP_NODELAY to the PipeImpl constructor and it should 
be a lot cleaner. It also means the "buffered" field is not needed.
- if you rename tryUnixDomain noUnixDomainSockets you avoid the 
volatile-write at initialization time
- createListener would be a clean if the flag is checked before the 
try/catch.

src/java.base/windows/native/libnet/net_util_md.h
- what is the include of afunix.h needed, left over?

src/java.base/windows/native/libnio/ch/Net.c
- are these changed needed? I would only checked localInetAddress and 
remoteInetAddress to be called on file descriptor to inet sockets.

src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
- IOUtil.makePipe return a long that encodes the two file descriptors 
and may be simpler to avoid populating the int[] in JNI code.

test/jdk/com/sun/nio/sctp/SctpServerChannel/NonBlockingAccept.java
- did you mean to change this test?

test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/ConnectionPoolTest.java
- is this needed?

I'm still working through new Unix* code and the tests so more comments 
soon.

-Alan