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

[Alan Bateman]

On Thu, 30 Dec 2021 10:12:20 GMT, Markus KARG <duke at openjdk.java.net> wrote:

>>> @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?

@mkarg Do you have any benchmarks/results for the change? I'm asking because FileChannel transferTo is implemented (where possible) on sendfile/equivalent so fast for the file -> socket and file -> file cases. The changes in this PR make use of FileChannel transferFrom so may not be significantly better than the default implementation.

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

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