RFR 8231187: SelectorProvider.inheritedChannel() returns TCP socket channel for Unix domain socket

[Michael McMahon]

Hi Chris,

On 24/09/2019, 10:17, Chris Hegarty wrote:
> Michael,
>
>> On 23 Sep 2019, at 19:04, Michael McMahon 
>> <michael.x.mcmahon at oracle.com <mailto:michael.x.mcmahon at oracle.com>> 
>> wrote:
>>
>> On 23/09/2019, 18:32, Alan Bateman wrote:
>>> On 23/09/2019 17:56, Michael McMahon wrote:
>>>> Okay, I see the reason for it now. It is the potential race when 
>>>> closing a socket
>>>> and its file descriptor possibly being reassigned to another opened 
>>>> file, at the same time
>>>> as another thread starting an I/O operation on the original socket.
>>>>
>>>> I have added the two stage close logic similar to SocketChannel and 
>>>> the Source/SinkChannels you
>>>> mentioned.
>>>>
>>>> Updated webrev at: 
>>>> http://cr.openjdk.java.net/~michaelm/8231187/webrev.2/index.html 
>>>> <http://cr.openjdk.java.net/%7Emichaelm/8231187/webrev.2/index.html>
>
>
> This looks good. A few comments..
>
>   - I only see one usage of private UnixDomainSocketChannelImpl::getFD
>   accessor, while other usages deference the field directly. Can getFD
>   be removed, or why is that usage different?
>
Yes, getFD was previously package private so used externally as well.
It can be removed completely at this point.
>   - I'm a little confused by the following, in 
> UnixDomainSocketChannelImpl:
>
>     156             } finally {
>     157                 endRead(n > 0);
>     158                 if (n <= 0)
>     159                     return IOStatus.EOF;
>     160             }
>     161             return IOStatus.normalize(n);
>
>     Is it always the case that if `n` is less than or equal to zero,
>     that the socket is at EOF? what about errors?  If the former is
>     true, then there is no need to normalize `n`, it is guaranteed to be
>     positive, no?
>
Hmm. Yes, I think the two lines 158:159 are redundant and
also the normalize call at 161, and the equivalent call in the write method.
This code originated obviously in classes that need to support the 
non-blocking
case where the relevant return codes may need to be normalized.

   - In UnixDomainSocketChannelImpl(..)
>
>      69     UnixDomainSocketChannelImpl(FileDescriptor fd)
>      70         throws IOException
>      71     {
>      72         this.fd = fd;
>      73         this.state = ST_INUSE;
>      74     }
>
>    The setting of the state here is, without holding the stateLock,
>    raises questions. But it should be safe since ST_INUSE is the
>    primitive's default initial value, `0`. If that were to change, then
>    not holding the lock could be problematic. Maybe just grab the lock.
>
Or just do what the related classes do, and not bother to explicitly 
initialize
in the constructor.

>   - In UnixDomainSocketChannelImpl:
>
>    170      * @throws ClosedChannelException if the channel is closed 
> or output shutdown
>
>    output cannot be shutdown - please remove "or output shutdown"
>
>

OK

>   - Maybe update the copyright year ranges in the existing files

Right, yes.

Thanks
Michael.
>
> -Chris.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190924/5efbd273/attachment-0001.html>