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/2d7ca1fc...7f677d5a

test/jdk/java/net/UnixDomainSocketAddress/LengthTest.java line 68:

> 66:                 {new String(new char[100]).replaceAll("\0", "x")},
> 67:                 {new String(new char[namelen]).replaceAll("\0", "x")},
> 68:                 {new String(new char[namelen-1]).replaceAll("\0", "x")},

What's the story with `namelen` ? AFAICS it is always equals to 100? Is it some left over?

test/jdk/java/nio/channels/unixdomain/Bind.java line 175:

> 173:         checkNormal(() -> {
> 174:             client = SocketChannel.open(StandardProtocolFamily.UNIX);
> 175:             client.bind(null);

Should there be a test that also verify that you could call:
client.bind(UNNAMED);
AFAIU that should be equivalent to `client.bind(null)` ?
(same remark for the server side)

test/jdk/java/nio/channels/unixdomain/Bind.java line 245:

> 243:                 client = SocketChannel.open(StandardProtocolFamily.UNIX);
> 244:                 client.bind(cAddr);
> 245:                 client.bind(cAddr);

Should there be a similar test where the first bind call is `client.bind(null)` ?
Same for the server side?

Also maybe there should be a test where the two addresses are different?

test/jdk/java/nio/channels/unixdomain/Bind.java line 178:

> 176:             SocketAddress a = client.getLocalAddress();
> 177:             assertAddress(a, nullAddr, "null address");
> 178:             assertEquals(a, UNNAMED);

If after binding, the client's localAddress  is still unnamed, and if the file is not automatically deleted, then how
do you find out which file to delete when you have finished with the socket? (I mean, in a real world application - not
in this test). Or am I missing something?

test/jdk/java/nio/channels/unixdomain/Security.java line 64:

> 62:             threw = true;
> 63:             if (!(expectedException.isAssignableFrom(t.getClass()))) {
> 64:                 throw new RuntimeException("wrong exception type thrown " + t.toString());

Maybe the wrong exception should be set at the cause to help debugging if the test ever fails here.

test/jdk/java/nio/channels/unixdomain/Security.java line 69:

> 67:         if (expectedException != null && !threw) {
> 68:             // should have thrown
> 69:             throw new RuntimeException("SecurityException was expected");

Well technically the message should say
throw new RuntimeException("%s was expected".formatted(expectedException.getName()));

test/jdk/java/nio/channels/unixdomain/Security.java line 78:

> 76:            SocketChannel.open(UNIX);
> 77:         }
> 78:         catch (UnsupportedOperationException e) {

Any reason for the strange formatting?

test/jdk/java/nio/channels/unixdomain/Security.java line 48:

> 46:
> 47: public class Security {
> 48:

I was expecting to see a test case that verifies that the local address is not revealed if the caller doesn't have the
required permission. Maybe it's already tested elsewhere?

test/jdk/java/nio/channels/unixdomain/Shutdown.java line 3:

> 1: /*
> 2:  * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Should this be 2020?

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

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