RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

[Brian Burkhalter]

On Thu, 29 Jul 2021 08:12:23 GMT, Markus KARG <github.com+1701815+mkarg at openjdk.org> wrote:

>> src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179:
>> 
>>> 177:             for (long n = srcSize - srcPos; bytesWritten < n;)
>>> 178:                 bytesWritten += src.transferTo(srcPos + bytesWritten, Long.MAX_VALUE, dest);
>>> 179:             return bytesWritten;
>> 
>> If `src` is extended at an inconvenient point in time, for example between the return from a call to `src.transferTo()` that makes `bytesWritten < n` false and the evaluation of that terminating condition, then it appears that not all the content of `src` will be transferred to `dest`. I am not however sure that this violates any contract but it could be a behavioral change from the existing implementation.
>
> The JavaDocs in `InputStream::transferTo` *cleary* tell the caller that there is **no** guarantee of *any* specific behavior in that particular case: 
>>The behavior for the case where the input and/or output stream is asynchronously closed, or the thread interrupted during the transfer, is highly input and output stream specific, and therefore not specified.

Good point.

>> src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85:
>> 
>>> 83:     private byte[] bs;       // Invoker's previous array
>>> 84:     private byte[] b1;
>>> 85: 
>> 
>> It might be better to put the field declarations at the beginning of the class before the static methods.
>
> This is a question of style and personal likes. Code maintenance is much easier if variables are defined *near* to its first use, not *far away* (as in the Pascal or C language). If the reviewers want me to change it, I will do it.

It's actually a matter of convention but I think it can remain as it is.

>> test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67:
>> 
>>> 65:         test(readableByteChannelInput(), defaultOutput());
>>> 66:     }
>>> 67: 
>> 
>> This test looks like it's doing what it's supposed to do. I'm not asking to change it, but I think using TestNG might have given a simpler result with less work. For example, there could be one `DataProvider` supplying inputs which feed a `@Test` which expects an NPE, and another `DataProvider` supplying input-output pairs which feeds a `@Test` which validates the functionality.
>
> No doubt, using modern tools would have spared me work, and indeed I would have chosen JUnit or TestNG if there would be a clear commitment to that tool *upfront*. For now, I see now use in reworking the code afterwards.

No need to rework it now.

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

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