RFR: 8278268 - (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets [v15]

[Markus KARG]

On Wed, 29 Dec 2021 07:47:58 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>>> Looking at the fact that selectableChannelOutput() utilizes Channels.newInputStream() it comes to my mind that [Trisha Gee reported](https://trishagee.com/2021/12/21/simple-network-connections-with-java-a-problem/) that Channels.newInputStream() produced an endless hanging of her apparently correct code, and it was gone once she replaced it by Channels.newReader(). While she reproduced that on a Mac, it might be the same cause even on Windows. Trisha currently is working on a reproducer, but she already detected that _her_ endless hanging is definitively a thread waiting for the blocking lock. This sounds similar to what you wrote above. So maybe the problem is not inside of TransferTo, but inside of ChannelInputStream?
>> 
>> I think the hang in JDK-8278369 is specific to the Unix domain socket implementation of Pipe on Windows 10+ and Windows Server 2019+. More specifically, I think the issue is closing the sink after a large number of bytes has been written does not always cause a reader on the source side to wakeup with EOF. As a test, I changed the implementation to use TCP sockets unconditional (so it works like Windows 8 or Windows Server 2012 or 2016) and I cannot duplicate the issue. We need to look closer at this in the new year.
>> 
>> I looked briefly at the chat server and it has a number of issues.  There is a ArrayList mutated and read from several threads without synchronization. There are several issues that are possible when a chat client does not keep up, this will eventually cause the chat server to hang (a thread blocked on the PrintWriter) and all other threads will eventually blocking trying to echo the messages. So from a quick look, I don't think it is anything to do with the issues we are discussing here.
>
>> @AlanBateman I found [the cause of the problem](https://github.com/trishagee/socket-locking/pull/1#issuecomment-1002187644). The problem is that ChannelInputStream and ChannelOutputStream share the same blocking lock when created for the same IP connection, which is pretty straightforward, as a SocketChannel is bidirectional by nature, but has only one blocking setting for both directions. This shared lock is not documented anywhere, so how should an application programmer know about this? This should _either_ be documented in the Channels JavaDocs, or (even better) there should be separate blocking modes per direction. FYI, the channel reader does not produce the deadlock as it does not care about the blocking mode at all but simply expects non-blocking mode so it runs in an active loop (squandering power). Maybe we should align both implementations, reader and stream? WDYT?
> 
> Good sleuthing! This is a design bug that goes back to JSR-51/Java 1.4 (https://bugs.openjdk.java.net/browse/JDK-4509080). SelectableChannel specifies blockingLock, which is okay for adaptors and the like when they are readable or writable, but not both.  The socket adaptors have been re-implemented so as not to require the blockingLock during read/write operations so this limits the issue to the interop support that Channels provides. It's fixable with a bit of surgery. A first attempt would be for Channels.newInputStream to return sc.socket().getInputStream() when the channel is a SocketChannel. This requires dealing with cases where someone attempts to get an input stream from an unconnected or closed channel.  A second attempt would be to add sun.nio.ch.SocketInputStream
> 
> package sun.nio.ch;
> 
> import java.io.IOException;
> import java.io.InputStream;
> 
> public class SocketInputStream extends InputStream {
>     private final SocketChannelImpl sc;
>     public SocketInputStream(SocketChannelImpl sc) {
>         this.sc = sc;
>     }
>     @Override
>     public int read() throws IOException {
>         byte[] a = new byte[1];
>         int n = read(a, 0, 1);
>         return (n > 0) ? (a[0] & 0xff) : -1;
>     }
>     @Override
>     public int read(byte[] b, int off, int len) throws IOException {
>         return sc.blockingRead(b, off, len, 0);
>     }
>     @Override
>     public int available() throws IOException {
>         return sc.available();
>     }
>     @Override
>     public void close() throws IOException {
>         sc.close();
>     }
> }
> 
> The main question is whether it's worth doing.

@AlanBateman @LanceAndersen Back to the original thread of this PR, are there more change requests or is it fine for merging?

-------------

PR: https://git.openjdk.java.net/jdk/pull/6711