RFR: 8245194: Unix domain socket channel implementation

Tue Sep 29 07:13:17 UTC 2020

On Fri, 11 Sep 2020 14:41:32 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.
>>>
>>>
>>>
>>>
>>>
>>>
>
>> _Mailing list message from [Alan Bateman](mailto:Alan.Bateman at oracle.com) on
>> [nio-dev](mailto:nio-dev at openjdk.java.net):_
>> On 07/09/2020 13:14, Michael McMahon wrote:
>> 
>> > > 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.
>> 
>> The file system provider shouldn't know anything about Unix domain
>> sockets so I prefer not to have "UnixDomain" in the name of this method.
>> I also would prefer if null is an exception so that it is consistent
>> with the other methods.
>> 
> 
> I'm not sure what to call the method then. It returns a UTF-8 string
> converted to bytes on Windows and the output of getByteArrayForSysCalls on Unix.
> It could be called getByteArrayUTF8 on Windows, but that would not
> be right on Unix.
> 
> Since sockets are file system entities, why not have socket in the method name?
> 
> What about getByteArrayForSocket() ?
> 
>> I think it would be useful to also summarize how the bind/connect works
>> on Windows. For example, suppose the working directory is 240 characters
>> and the UnixDomainSocketAddress is to a simple relative path of 20
>> characters. If I understand correctly, the proposal will encode the
>> simple relative path (not the resolved absolute path) to bytes using
>> UTF-8 so it will probably be 20+ bytes in this case.
> 
> Right
> 
>> This hould be okay
>> but inconsistent with the rest of? file system implementation which will
>> use the long form. Also if the name is long then it won't use the long
>> path prefix (\?) but instead rely on failure because the resulting
>> sequence of bytes when encoded is > 100, is that correct?
>> 
> 
> Yes, that is how it is working currently. We check the length in native code
> just before calling bind(). Whether the long path prefix is present or not
> won't matter because the path will not be used once it is longer than ~100 bytes.
> 
> Michael.
>> -Alan.

Hi Alan,

Thanks for the comments. Assume all are accepted, except as queried below

> _Mailing list message from [Alan Bateman](mailto:Alan.Bateman at oracle.com) on
> [nio-dev](mailto:nio-dev at openjdk.java.net):_
> On 24/09/2020 13:17, Michael McMahon wrote:
> 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.

struct sockaddr_un is declared in <afunix.h> on Windows and <sys/un.h>
on Unix. So, it is included via net_util_md.h

I suspect the definition is only in fairly recent SDKs on Windows, and I'm not sure
what I can do about that. If <afunix.h> is present
it will have the definition. If it's not present, then there will be a compile
error, but I don't know how to tell if it is present..

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

The problem here is that the Java impl of UnixDomainHelper is platform
specific, but UnixDomainSockets is shared, but with platform specific
native methods. If I make UnixDomainSockets.java platform specific then
there will be a lot of duplication of code. Maybe we could pick a better name
than UnixDomainHelper?

As regards hard-coding of /tmp and /var/tmp, I think we need to be very
careful to ensure that the directory name used to hold these automatically
bound sockets is as short as possible. ${java.io.tmpdir} is sometimes
long enough on our test systems to push the names over the ~100 byte limit.
I don't see a good alternative to using /tmp or /var/tmp on Unix
On Windows %TEMP% seems to be always defined and seems to always
refer to a directory with a short name.

What about "jdk.nio.unixdomain.tmpdir" for the property name?

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

Explained above <afunix.h> is Windows only.

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

I had changed this file, but reverted all changes in the last version. I don't
know why the webrev is still seeing changes. After this round, hopefully
they will be gone.

> 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

Thanks for the review. I'm making the changes as described, except where noted
above for further discussion. This should appear as a new webrev revision today.

Michael

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

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