RFR 8245194: Unix domain socket channel implementation

[mark sheppard]

Hi Michael,
  I see that you have removed the getProtocolFamily method so the following suggestion may be
a moot point.
  In any case, one possible alternative approach for providing access to the ProtocolFamily attribute of
a {Server}SocketChannel, might be via a (socket) option type for the  getOption method, e.g.
SO_PROTOCOL_FAMILY ?  This is analogous to the SO_TYPE at the native OS system socket api level.
Might be worth considering, as previously there was some suggested value in having access to such information.

regards
Mark

________________________________
From: nio-dev <nio-dev-retn at openjdk.java.net> on behalf of Michael McMahon <michael.x.mcmahon at oracle.com>
Sent: Tuesday 21 July 2020 11:58
To: Alan Bateman <Alan.Bateman at oracle.com>; nio-dev at openjdk.java.net <nio-dev at openjdk.java.net>
Subject: Re: RFR 8245194: Unix domain socket channel implementation

Alan

I have updated the API webrev [1], impl webrev [2] and specdiff [3]. A
couple of things I reconsidered since my last reply:

- I have removed the paragraphs relating to SocketChannel and
ServerSocketChannel behavior for Unix Domain from the package-info and
added a small amount of related text to the @apiNote to both classes'
bind methods

- I am thinking now the system property for overriding the temporary
socket file directory should not be part of the Java SE spec. So, it
does not now appear anywhere in the api doc. We will probably have to
choose a different name for it and decide where it could be documented
instead.

One thing I just noticed is that the NetPermission("unixDomainSocket")
looks a little out of place compared to the other permission targets in
that class as they all have a verb like "allow", or "get" prefixing the
name. So, I wonder if it might be better as "allowUnixDomainSocket"?

Thanks,

Michael


[1] http://cr.openjdk.java.net/~michaelm/8245194/api.webrev/webrev.6/

[2] http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.6/

[1] http://cr.openjdk.java.net/~michaelm/8245194/specdiff/webrev.6/

On 20/07/2020 15:38, Michael McMahon wrote:
>
> On 20/07/2020 10:16, Alan Bateman wrote:
>> On 17/07/2020 17:02, Alan Bateman wrote:
>>> On 17/07/2020 15:57, Michael McMahon wrote:
>>>> :
>>>>
>>>> I have added a new revision of the webrevs/specdiffs including
>>>> these and Daniel's comments at:
>>>>
>>>> [1] http://cr.openjdk.java.net/~michaelm/8245194/api.webrev/webrev.5/
>>> How tied are you to adding getProtocolFamily to these APIs? It's
>>> technically an incompatible change to add abstract methods to these
>>> classes although the number of implementations outside of the JDK is
>>> probably tiny. I guess I'm mostly wondering if the method is
>>> actually useful. I could imagine a testing checking to see if a
>>> channel is unbound and then binding it to a socket address that is
>>> appropriate for the channel's socket but I'm less sure about other
>>> uses. Are you mostly thinking diagnostics, in which case it can be
>>> in the string representation.
>> One other comment on getProtocolFamily() is that if it is added then
>> we should look at NetworkChannel as that's the interface that all
>> channels to a socket implement.
>>
> Right. I'm questioning now whether it is worth it. I don't see any
> compelling use cases at this point and I hadn't considered other
> potential implementations and what any default implementations of the
> method should return. I found it didn't improve the spec in the way I
> expected either. So, I propose to drop it at this point.
>> A few other comments on the API webrev (v5):
>>
>> NetPermission("allowUnixDomainChannels"). A coarse grain permission
>> is fine but I'm not sure about the target name. It's not strictly
>> tied to channels and the "allow" prefix is superfluous. You might
>> want to consider something like "unixDomainSocket" and just specify
>> that it is required in order to accept, bind, or connect a Unix
>> domain socket.
>>
> ok "unixDomainSocket" seems fine.
>> UnixDomainSocketAddress. The javadoc for the factory methods should
>> probably make it clear that the path can be empty. of(Path) might
>> also need something to cover the corner case that the default file
>> system is a custom file system (it will throw IAE for this case). The
>> hashCode method specifies that the hash code is based on the path
>> string where it's based on the Path object, might be simpler to just
>> specify that it returns the hash code of the UnixDomainSocketAddress.
>>
> Ok, except the static of(Path) method already specifies in the @throws
> that it throws IAE if the path comes from a filesystem other than the
> system default? Is that not sufficient?
>
>> The javadoc updates to SocketChannel and ServerSocketChannel look
>> okay to me. A few long length inconsistencies/formatting but these
>> are just nits.
>>
>> SelectorProvider. I don't think the list items need to repeat
>> "Internet protocol" because it's already in the paragraph for the
>> lists. Also just a minor point that there is no need for all the
>> javadoc links to use qualified class names as SelectorProvider
>> imports all these types.
>>
> OK
>> The API additions to the jdk.net package look good too.
>>
>> The first 3 paragraphs added to the java.nio.channels package
>> description are okay.
>>
>> I have a lot of comments on the additions to the java.nio.channel
>> package description. Some of these paragraphs don't make sense here,
>> e.g. paragraphs 4 and 5 candropped and replaced with an apiNote in
>> DatagramChannel to say that channels to datagram-oriented sockets do
>> not support Unix domain sockets. There is also a lot of text that is
>> too specific to SocketChannel and ServerSocketChannel so I think
>> we'll need to find a better place for this. I don't think there is
>> any need to reference to the security manager as it is already
>> covered by the API docs.
>>
> OK, I've made some of the updates above, but have not moved the
> paragraphs relating to SocketChannel/ServerSocketChannel, as I don't
> know where to move them to and I think
> the details are important.
>
>
>> Are you proposing that the java.nio.channels.tmpdir property be
>> standard or JDK specific? I think we need to know this before
>> suggesting a name for this property.
>>
> My suggestion would be Java SE as I think it will need to be quite
> visible.
>
>
>> Overall I think the API/docs are looking quite good, just not sure
>> about getProtocolFamily and the additions to the package description
>> will probably go through a few iterations.
>>
> Okay, thanks.
>
> - Michael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200722/e15a03c6/attachment.htm>