RFR 8245194: Unix domain socket channel implementation

[Michael McMahon]

Hi Mark


Thanks for reviewing the webrev.


I think that might be an option for providing the function at a JDK (non 
standard) level.

But, I don't currently see a strong enough justification for it.


Michael.


On 22/07/2020 17:15, mark sheppard wrote:
> 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/20200723/6d641a59/attachment-0001.htm>