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

[Daniel Fuchs]

On Tue, 13 Oct 2020 19:09:44 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

>> Continuing this review as a PR on github with the comments below incorporated. I expect there will be a few more
>> iterations before integrating.
>> On 06/09/2020 19:47, Alan Bateman wrote:
>>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>>
>>>> As I mentioned the other day, I wasn't able to use the suggested method on Windows which returns an absolute path. So,
>>>> I added a method to WindowsPath which returns the path in the expected UTF-8 encoding as a byte[]. Let me know what you
>>>> think of that.
>>>>
>>>> There is a fair bit of other refactoring and simplification done also. Next version is at:
>>>>
>>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>>
>>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I don't think we should be caching it as the
>>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about encoding a relative path when the resolved
>>> path (before encoding) is > 247 chars. The documentation on the MS site isn't very completely and I think there are a
>>> number points that require clarification to fully understand how this will work with relative, directly relative and
>>> drive relative paths.
>>>
>> 
>> Maybe it would be better to just use the path returned from toString() and do the conversion to UTF-8 externally. That
>> would leave WindowsPath unchanged.
>>> In the same area, the new PathUtil is a bit inconsistent with the existing provider code. One suggestion is to add a
>>> method to AbstractFileSystemProvider instead. That is the base class the platform providers and would be a better place
>>> to get the file path in bytes.
>>>
>> 
>> Okay, I gave the method a name that is specific to Unix domain sockets because this UTF-8 strangeness is not likely to
>> be useful by other components.
>>> One other comment on the changes to the file system provider it should be okay to change UnixUserPrinipals ad its
>>> fromUid and fromGid method to be public. This would mean that the patch shouldn't need to add UnixUserGroup (the main
>>> issue is that class is that it means we end up with two classes with static factory methods doing the same thing).
>> 
>> Okay, that does simplify it a bit.
>> 
>> Thanks,
>> Michael.
>> 
>>> -Alan.
>>>
>>>
>>>
>>>
>>>
>>>
>
> 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 22 additional commits since
> the last revision:
>  - Merge branch 'master' into unixdomainchannels
>  - - reorganised the channel impls back into SocketChannelImpl and ServerSocketChannelImpl
>    - removed the new Unix domain socket events and folded the behavior into the existing socket events
>    - implemented other comments from Alan on Oct 11.
>  - unixdomainchannels: updates from Chris's review 9 Oct 2020
>  - unixdomainchannels:
>    - updated property name
>    - added JFR unit test
>  - Merge branch 'master' into unixdomainchannels
>  - - update after Alan's review on Oct 4
>    - includes API change required by JDK-8251952
>    - original CSR for the overall change will be resubmitted with
>      all api changes consolidated based on this update
>  - - simplified Copy.gmk to CAT source files directly
>    - renamed net.properties source files to all be net.properties
>  - unixdomainchannels: error in the last commit in make/modules/java.base/Copy.gmk
>  - unixdomainchannels:
>    (1) rename UnixDomainHelper to UnixDomainSocketsUtil
>    (2) remove hardcoded /tmp and /var/tmp paths from UnixDomainSocketsUtil
>    (3) replace (2) with documented system/networking properties
>    (4) Small update to UnixDomainSocketAddress API
>    (5) CSR for (3) and (4) submitted at JDK-8253930
>    (6) Update build to generate net.properties from shared net.properties.common
>        plus platform specific additions.
>  - Merge branch 'master' into unixdomainchannels
>  - ... and 12 more: https://git.openjdk.java.net/jdk/compare/bc3cab15...7f677d5a

src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 63:

> 61:     /** Return the, possibly empty, set of extended socket options available. */
> 62:     public final Set<SocketOption<?>> unixOptions() { return unixOptions; }
> 63:

Could the name of the method - or alternatively its API doc comment - be improved to make it clear that these options
are **Unix Domain protocol** specific option (as opposed to Unix OS specific option). For instance, the field and
method could be named: `unixDomainOptions` instead of `unixOptions`?

src/java.base/share/classes/sun/nio/ch/ServerSocketAdaptor.java line 96:

> 94:     @Override
> 95:     public InetAddress getInetAddress() {
> 96:         InetSocketAddress local = (InetSocketAddress)ssc.localAddress();

Idle wondering as to whether ServerSocketChannelImpl could be parameterized to avoid casts...
`... class ServerSocketChannelImpl<SA extends SocketAddress> extends ...`
Or maybe that would just make things more complex. Have you considered it and rejected it?

src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 99:

> 97:     // Binding
> 98:     private SocketAddress localAddress; // null => unbound
> 99:

See my previous comment about parameterization.

src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 267:

> 265:             HashSet<SocketOption<?>> set = new HashSet<>();
> 266:             set.add(StandardSocketOptions.SO_RCVBUF);
> 267:             return Collections.unmodifiableSet(set);

Wondering: should there be a call to `ExtendedSocketOptions` after line 266 here, even if that call just returns an
empty set?

src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 430:

> 428:
> 429:     protected int implAcceptUnix(FileDescriptor fd, FileDescriptor newfd, SocketAddress[] addrs)
> 430:         throws IOException

Does this method needs to be `protected` ? I couldn't find a place where it was called outside of this file. By
contrast `implAcceptInet` was declared `private` above...

src/java.base/share/classes/sun/nio/ch/SocketAdaptor.java line 110:

> 108:     public InetAddress getInetAddress() {
> 109:         InetSocketAddress remote = (InetSocketAddress)sc.remoteAddress();
> 110:         if (remote == null) {

I guess I'll have the same question about parameterizing `SocketChannelImpl`. Was it considered and rejected?

src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 152:

> 150:         this.family = family;
> 151:         this.fdVal = IOUtil.fdVal(fd);
> 152:     }

If you introduced:
private static FileDescriptor socketFor(ProtocolFamily family) throws IOException ...
and
private SocketChannelImpl(SelectorProvider sp, ProtocolFamily family, FileDescriptor fd) throws IOException {
      super(sp);
      this.family = family;
      this.fd = fd;
      this.fdVal = IOUtil.fdVal(fd);
 }
then you could move all the checking to `socketFor(ProtocolFamilly)` and write this constructor as:
SocketChannelImpl(SelectorProvider sp, ProtocolFamily family) throws IOException {
    this(sp, family, sockectFor(family));
}
which would push all the validation to **before** the object gets constructed.

src/java.base/unix/native/libnio/ch/UnixDomainSockets.c line 65:

> 63:         if (namelen != 0) {
> 64:             (*env)->SetByteArrayRegion(env, name, 0, namelen, (jbyte*)sa->sun_path);
> 65:         }

Should the exception status be checked after calling `SetByteArrayRegion`?

src/java.base/unix/native/libnio/ch/UnixDomainSockets.c line 76:

> 74:     sa->sun_family = AF_UNIX;
> 75:     int ret;
> 76:     const char* pname = (const char *)(*env)->GetByteArrayElements(env, path, NULL);

Should `pname == NULL` be checked?

src/java.base/windows/native/libnio/ch/UnixDomainSockets.c line 48:

> 46:         if (name != NULL) {
> 47:             (*env)->SetByteArrayRegion(env, name, 0, namelen, (jbyte*)sa->sun_path);
> 48:         }

Same remark here - about exception status

src/java.base/windows/native/libnio/ch/UnixDomainSockets.c line 66:

> 64:     jboolean isCopy;
> 65:     char *pname = (*env)->GetByteArrayElements(env, addr, &isCopy);
> 66:

... and here about checking for `pname == NULL`

-------------

PR: https://git.openjdk.java.net/jdk/pull/52