Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8245194: Unix domain socket channel implementation #52

Closed

Conversation

Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Sep 7, 2020

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8245194: Unix domain socket channel implementation

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52
$ git checkout pull/52

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 7, 2020

👋 Welcome back michaelm! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 7, 2020
@openjdk
Copy link

openjdk bot commented Sep 7, 2020

@Michael-Mc-Mahon The following labels will be automatically applied to this pull request: core-libs hotspot-jfr net nio.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org labels Sep 7, 2020
@Michael-Mc-Mahon
Copy link
Member Author

/csr needed

@openjdk
Copy link

openjdk bot commented Sep 9, 2020

@Michael-Mc-Mahon the issue for this pull request, JDK-8245194, already has an approved CSR request: JDK-8247942

…automatically

bound server sockets are created. After this change it is no longer necessary
to specify the location in individual tests.
@mlbridge
Copy link

mlbridge bot commented Sep 11, 2020

Mailing list message from Alan Bateman on nio-dev:

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

-Alan.

@Michael-Mc-Mahon
Copy link
Member Author

Mailing list message from Alan Bateman on nio-dev:

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.

@mlbridge
Copy link

mlbridge bot commented Sep 27, 2020

Mailing list message from Alan Bateman on nio-dev:

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

@Michael-Mc-Mahon
Copy link
Member Author

Hi Alan,

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

Mailing list message from Alan Bateman on nio-dev:

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

(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.
@openjdk openjdk bot added the build build-dev@openjdk.org label Oct 2, 2020
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 20, 2020
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 20, 2020
dfuch
dfuch approved these changes Oct 20, 2020
dfuch
dfuch approved these changes Oct 21, 2020
test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved
test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved
- test updates from Chris
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Oct 27, 2020
@Michael-Mc-Mahon
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Oct 28, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 28, 2020
@openjdk
Copy link

openjdk bot commented Oct 28, 2020

@Michael-Mc-Mahon Since your change was applied there have been 29 commits pushed to the master branch:

  • 8bde2f4: 8255013: implement Record Classes as a standard feature in Java, follow-up
  • 0425889: 8255429: Remove C2-based profiling
  • aaf4f69: 8255233: InterpreterRuntime::at_unwind should be a JRT_LEAF
  • bbf0a31: 8255397: x86: coalesce reference and int entry points into vtos bytecodes
  • 3bd5b80: 8243583: Change 'final' error checks to throw ICCE
  • 1f00c3b: 8255527: Shenandoah: Let ShenadoahGCStateResetter disable barriers
  • 3c4fc79: 8255299: Drop explicit zeroing at instantiation of Atomic* objects
  • 6b2d11b: 8255246: AArch64: Implement BigInteger shiftRight and shiftLeft accelerator/intrinsic
  • 591e7e2: 8255378: [Vector API] Remove redundant vector length check after JDK-8254814 and JDK-8255210
  • 2c9dfc7: 8255234: ZGC: Bulk allocate forwarding data structures
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/dccfd2b3e0636488e255da1fa5b1b58e9bc1b606...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6bb7e45.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@Michael-Mc-Mahon Michael-Mc-Mahon deleted the unixdomainchannels branch October 28, 2020 18:11
cushon pushed a commit to cushon/jdk that referenced this pull request Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-jfr hotspot-jfr-dev@openjdk.org integrated Pull request has been integrated net net-dev@openjdk.org nio nio-dev@openjdk.org
5 participants