Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Tue, 13 Oct 2020 19:09:44 GMT, Michael McMahon 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> 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(
Re: RFR: 8245194: Unix domain socket channel implementation [v18]
On Fri, 9 Oct 2020 14:55:25 GMT, Michael McMahon 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:
>
> unixdomainchannels: updates from Chris's review 9 Oct 2020
Very good work. I have not looked at the tests yet.
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 134:
> 132: if (fs.getClass().getModule() != Object.class.getModule()) {
> 133: throw new IllegalArgumentException();
> 134: }
I believe it would be better if this verification was done outside of the
constructor (you could do it in the of(Path)
static factory method) - this would avoid creating the object if the path is
invalid.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 11:04:08 GMT, Daniel Fuchs wrote:
>> 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/21a8f700...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> 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`?
I think unixDomainOptions would be reasonable and the docs could be updated
also to clearly define what each set is.
> 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 extends ...`
> Or maybe that would just make things more complex. Have you considered it and
> rejected it?
Hmm. Not sure how much it would improve clarity, though it would remove some
annoying casts. Let me look into it.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v18]
On Tue, 13 Oct 2020 15:50:57 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> unixdomainchannels: updates from Chris's review 9 Oct 2020
>
> src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 134:
>
>> 132: if (fs.getClass().getModule() != Object.class.getModule()) {
>> 133: throw new IllegalArgumentException();
>> 134: }
>
> I believe it would be better if this verification was done outside of the
> constructor (you could do it in the of(Path)
> static factory method) - this would avoid creating the object if the path is
> invalid.
Good point.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 11:37:09 GMT, Daniel Fuchs wrote: >> 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/0017bc71...7f677d5a > > src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 267: > >> 265: HashSet> 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? I suppose for consistency that would make sense, though it would be an empty set for now. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v17]
On Fri, 9 Oct 2020 14:36:44 GMT, Michael McMahon wrote: >> test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 113: >> >>> 111: SPINBAccep_NBConn_NBIO_WR_11a >>> 112: SPINBAccep_NBConn_NBIO_RW_12a >>> 113: */ >> >> I recognise this test ;-) I thought there was a version for IP-specific >> channels, but cannot find it now. I was going >> to ask if these could be merged or abstracted out somehow. > > Chris, > Thanks for the comments. I will incorporate them all into the next revision. > As regards the IOExchanges test, it uses a > @DataProvider of ProtocolFamily with values "UNIX" and "INET" which means the > tests are run for TCP sockets. Is that > what you were asking? I'm not sure I get the question exactly. Michael. Should it also have INET6 in the case where IPv6 is supported on the machine? - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Tue, 13 Oct 2020 19:09:44 GMT, Michael McMahon 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 ser
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 13:17:01 GMT, Michael McMahon wrote:
>> 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> 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`?
>
> I think unixDomainOptions would be reasonable and the docs could be updated
> also to clearly define what each set is.
I changed it to unixDomainClientOptions to distinguish from server channel
options, which there are none anyway.
>> src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 267:
>>
>>> 265: HashSet> 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?
>
> I suppose for consistency that would make sense, though it would be an empty
> set for now.
Actually, related to the last comment I just decided to make it clear the
extended options are only for the client side
and it doesn't make sense to add all this code for a set of possible options
that will be empty.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 11:19:59 GMT, Daniel Fuchs wrote:
>> 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/a9828a5a...7f677d5a
>
> 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.
I think this change (parameterizing the SocketChannelImpl and Server) would be
too disruptive for the benefit it
provides.
> 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.
Good idea.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 12:24:11 GMT, Daniel Fuchs wrote:
>> 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/def74ac0...7f677d5a
>
> 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`?
I notice a lot of other code is not fussy about checking this, but it would
look safer to check and return NULL in this
case.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 12:28:04 GMT, Daniel Fuchs wrote: >> 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/1e93b2d3...7f677d5a > > 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? A UnixDomainSocketAddress should always always have a path associated and therefore a byte array should always be present, but again I think this is no harm for overall robustness. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 13:56:42 GMT, Daniel Fuchs wrote:
>> 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/fe5bf7d9...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?
No, it is testing a name length that is close to the maximum value. At one
point I had a system property that indicates
the actual maximum length on a platform, but that was removed.
> 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)
That would be true for the client side, and I'll add a test for it. But it is
not allowed to bind explicitly to the
unnamed address on the server side. It doesn't make sense because an unnamed
address means no socket file on the file
system.
-
PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v21]
> 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: src changes from Daniel's review. Tests will come later - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/7f677d5a..4bbbde32 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=20 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=19-20 Stats: 69 lines in 6 files changed: 41 ins; 13 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 14:14:48 GMT, Daniel Fuchs wrote: >> 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/c064114d...7f677d5a > > 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? so, testing multiple binds of the same socket, but using different addresses, which should also fail? I can add that no problem. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v20]
On Wed, 14 Oct 2020 14:21:13 GMT, Daniel Fuchs wrote: >> 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/918de17d...7f677d5a > > 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? The apiNote in SocketChannel.bind specifies that binding to an unnamed address does not create any socket file on the filesystem. I'd expect this to be normal behavior for clients, precisely so they dont have to worry about cleaning up the socket files after them. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v22]
> 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: - test updates from Daniel's review 14 Oct 2020 - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/4bbbde32..3859e611 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=21 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=20-21 Stats: 85 lines in 5 files changed: 75 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v23]
> 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: fix white space error - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/3859e611..8a545a7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=22 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=21-22 Stats: 33 lines in 1 file changed: 0 ins; 0 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v23]
On Wed, 14 Oct 2020 18:27:32 GMT, Michael McMahon 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:
>
> fix white space error
Marked as reviewed by dfuchs (Reviewer).
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 717:
> 715: UnixDomainSockets.bind(fd, path);
> 716: }
> 717: if (usa == null || path.toString().equals("")) {
Maybe you could replace `path.toString().length() > 0` with
`!path.toString().isEmpty()` and
`path.toString().equals("")` with `path.toString().isEmpty()`.
-
PR: https://git.openjdk.java.net/jdk/pull/52
