RFR 8245194: Unix domain socket channel implementation

[Michael McMahon]

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