RFR: 8276779: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels

[Alan Bateman]

On Thu, 19 Aug 2021 18:25:39 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>>> I had hoped that ThrowingLongSupplier would go away. If you keep your 2-arg transfer method then we should be able to implementation transferTo very simply, something like
>>> 
>>> ```
>>>         public long transferTo(OutputStream out) throws IOException {
>>>             if (ch instanceof FileChannel fc
>>>                     && out instanceof ChannelOutputStream cos) {
>>>                 WritableByteChannel target = cos.channel();
>>>                 if (target instanceof SelectableChannel sc) {
>>>                     synchronized (sc.blockingLock()) {
>>>                         if (!sc.isBlocking())
>>>                             throw new IllegalBlockingModeException();
>>>                         return transfer(fc, target);
>>>                     }
>>>                 }
>>>                 return transfer(fc, target);
>>>             }
>>>             return super.transferTo(out);
>>>         }
>>> ```
>> 
>> This is not true. It will only check the blocking mode for the target. But technically it is possible that someone writes an implementation of FileChannel which implements SelectableChannel. In that case it is needed to check the blocking mode on the source, too. So either we (a) ignore this possibility, (b) write a very complex code which calls transfer(fc, target) in FOUR code paths not just two, or (c) we keep the ThrowingLongSupplier as an elegant way to perform the 2x2 matrix.
>
>> This is not true. It will only check the blocking mode for the target. But technically it is possible that someone writes an implementation of FileChannel which implements SelectableChannel. In that case it is needed to check the blocking mode on the source, too. So either we (a) ignore this possibility, (b) write a very complex code which calls transfer(fc, target) in FOUR code paths not just two, or (c) we keep the ThrowingLongSupplier as an elegant way to perform the 2x2 matrix.
> 
> SelectableChannel is an abstract class, not an interface so you can't extend both FileChannel and SelectableChannel. So when the source channel is a FileChannel then it should only be concerned with whether the target channel is a SelectableChannel or not. I think we can keep it very simple, along the lines of the method I pasted in above.
> 
> If you choose to include the case where the target is a SelectableChannel (and I have no objection to doing that) then the test needs to exercise that code. This means the test needs an input stream created on a source file/FileChannel, and an output stream created an channel that is WritableByteChannel & SelectableChannel. A Pipe.SinkChannel or a connected SocketChannel will do fine. If you choose to leave out selectable channels for the first PR then file -> file is okay, which I think is what your current test is doing.

> @AlanBateman Kindly requesting review. :-)

The implementation change looks correct, it's as we discussed a few months ago: InputStream::transferTo needs to throw IllegalBlockingModeException when the source is an input stream backed by a FileChannel and the output stream is backed by a channel configured non-blocking. 

While not a change to existing behavior, I'm mulling over whether we should clarify the specification of Channels.newInputStream. The current spec is:

"The read methods of the resulting stream will throw an IllegalBlockingModeException if invoked while the underlying channel is in non-blocking mode. "

We can expand this to "The read and transferTo methods ...".

We could also add "The transferTo method will also throw an IllegalBlockingModeException if invoked to transfer bytes to an output stream that writes to an underlying channel in non-blocking mode".

I'm not saying we have to update the API docs as part of this PR, I'm just pointing out that the existing spec wasn't updated when InputStream.transferTo was added, it probably should have been.

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

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