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

[Daniel Fuchs]

On Mon, 19 Oct 2020 22:13:40 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 incrementally with one additional commit since the last revision:
> 
>   final feedback from Alan

The tests mostly look good to me. There's a general cleanup needed to make sure channels get closed properly in case of
exception (that is - use try-with-resources whenever possible).

test/jdk/java/net/UnixDomainSocketAddress/AddressTest.java line 43:

> 41: public class AddressTest {
> 42:
> 43:     static UnixDomainSocketAddress addr;

This static seems to be unused?

test/jdk/java/net/UnixDomainSocketAddress/AddressTest.java line 29:

> 27:  * @compile ../../nio/file/spi/TestProvider.java AddressTest.java
> 28:  * @run testng/othervm AddressTest
> 29:  */

Could add `@summary` - IIUC this test checks that a Path that doesn't come from the build in filesystem is rejected by
`UnixDomainSocketAddress`?

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 85:

> 83:         sc2.close();
> 84:         Files.delete(addr.getPath());
> 85:         ssc.close();

Should this use nested try-with-resources to close `ssc` and `sc2`?

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 107:

> 105:         sc2.close();
> 106:         ssc.close();
> 107:         return sc1;

Here again nested try-with-resources could be used to close sc2 and ssc.

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 136:

> 134:         ssc.close();
> 135:         InetSocketAddress isa = new InetSocketAddress(InetAddress.getLocalHost(), port);
> 136:         return SocketChannel.open(isa);

dito.

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 144:

> 142:         var addr = ssc.getLocalAddress();
> 143:         launch(className, null, null, Util.getFD(ssc));
> 144:         ssc.close();

Use try-with-resource?

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/Launcher.java line 170:

> 168:                                                             String[] options,
> 169:                                                             String... args)
> 170:             throws IOException {

Using try-with-resource or try-finally to make sure `dc` is closed properly should be envisaged.

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/StateTest.java line 172:

> 170:         /*
> 171:          * Launch service with a SocketChannel (tcp nowait)
> 172:          */

try-with-resources or try-finally could be used in this file too to ensure that everything gets properly closed.

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixDomainChannelTest.java line 56:

> 54:                     ByteBuffer buf = ByteBuffer.wrap(result.getBytes(ISO_8859_1));
> 55:                     sc.write(buf);
> 56:                 }

Should the test fail if `channel` is not an instance of `SocketChannel`? Or at least print some kind of debug
information? Do we want to verify that it's a Unix Domain Socket Channel - e.g. by looking at the local address?
Same question for the server socket case below...

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixDomainChannelTest.java line 78:

> 76:     // Test with a named connected socket
> 77:     private static void test1() throws Exception {
> 78:         ServerSocketChannel listener = ServerSocketChannel.open(UNIX);

Maybe try with resources or try-finally could be used to ensure that all channels are closed.

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 53:

> 51:     public static class Child2 {
> 52:         public static void main(String[] args) throws Exception {
> 53:             ServerSocketChannel server = (ServerSocketChannel)System.inheritedChannel();

Maybe use try-with-resources

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 41:

> 39:     public static class Child1 {
> 40:         public static void main(String[] args) throws Exception {
> 41:             SocketChannel chan = (SocketChannel)System.inheritedChannel();

try-with-resources?

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel/UnixSocketTest.java line 68:

> 66:
> 67:     public static void main(String args[]) throws Exception {
> 68:         SocketChannel sc = Launcher.launchWithUnixSocketChannel("UnixSocketTest$Child1");

try-with-resources?

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

> 116:                 client.close();
> 117:             if (accept1 != null)
> 118:                 accept1.close();

Maybe each of these `close()` calls should be in its own try-catch.

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

> 130:     static void assertAddress(SocketAddress a, UnixDomainSocketAddress a1, String s) {
> 131:         if (!(a instanceof UnixDomainSocketAddress)) {
> 132:             throw new RuntimeException("wrong address type");

It would be useful to which wrong address type was observed.

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

> 134:         UnixDomainSocketAddress ua = (UnixDomainSocketAddress)a;
> 135:         if (!a.equals(a1))
> 136:             throw new RuntimeException("this is not the " + s + " address");

Could you consider turning this test into a `testng` test so that asssertEquals can be used?
Alternatively if you want to retain the ability to run the test as a standalone java program out of the repository then
consider modifying the assertXxxx methods so that the exception message prints both parts: `<what>` is not equal to
`<what>`

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

> 138:
> 139:     static void assertEquals(Object a, Object b) {
> 140:         if (!a.equals(b))

Should this call Objects.equals instead?

test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java line 61:

> 59:             System.out.println("The socketChannel is : expected Null " + socketChannel);
> 60:             // exception could be thrown otherwise
> 61:         }

Do we have/need a test that goes the whole way down? Register for OP_CONNECT with a selector and wait for the connect
to finish?

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

> 125:         final UnixDomainSocketAddress saddr = UnixDomainSocketAddress.of(servername);
> 126:         final ServerSocketChannel server = ServerSocketChannel.open(UNIX);
> 127:         final SocketChannel client = SocketChannel.open(UNIX);

Maybe try-with-resources should be used and a finally too to ensure that the file is deleted...

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

> 143:         final UnixDomainSocketAddress saddr = UnixDomainSocketAddress.of(servername);
> 144:         final ServerSocketChannel server = ServerSocketChannel.open(UNIX);
> 145:         final SocketChannel client = SocketChannel.open(UNIX);

Same remark as above

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

> 161:         s1.bind(saddr);
> 162:         var s2 = ServerSocketChannel.open(UNIX);
> 163:         s2.bind(null);

same remark as above

test/jdk/java/nio/channels/unixdomain/SocketOptions.java line 59:

> 57:         s.bind(null);
> 58:         UnixDomainSocketAddress addr = (UnixDomainSocketAddress)s.getLocalAddress();
> 59:         SocketChannel c = SocketChannel.open(addr);

try-with-resources?

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

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