Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 20:21:10 GMT, Vladimir Sitnikov wrote: >>> What do you think? >> >> I think that you are looking at an obsolete repository: >> >> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802 > > I mean SequenceInputStream, not the base class: > https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249 > > Could you please double-check? The solution proposed by @vlsi and outlined by @bplb in [JDK-8322141](https://bugs.openjdk.org/browse/JDK-8322141) now can be reviewed in https://github.com/openjdk/jdk/pull/17119. I suggest to continue the discussion *there*. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427692263
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 23:17:41 GMT, Brian Burkhalter wrote: >> IMHO @vlsi is right. It is incorrect that we stop the transfer in the >> overflow case. We should fix it as he suggested. I can do that if you like. > > Right, the base class. The suggested change was made for `InputStream` in > 254d6990bcf75700f1c3aa18e0f8b73b639d9bad. It looks like it should be in > `SequenceInputStream` as well. I created > [JDK-8322141](https://bugs.openjdk.org/browse/JDK-8322141) to track this. We don't have any tests to check the behavior of the transferTo overrides when they transfer more than nine quintillion bytes so dependent on catching issues like this during code review. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427679131
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 21:54:53 GMT, Markus KARG wrote: >> I mean SequenceInputStream, not the base class: >> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249 >> >> Could you please double-check? > > IMHO @vlsi is right. It is incorrect that we stop the transfer in the > overflow case. We should fix it as he suggested. I can do that if you like. Right, the base class. The suggested change was made for `InputStream` in 254d6990bcf75700f1c3aa18e0f8b73b639d9bad. It looks like it should be in `SequenceInputStream` as well. I created [JDK-8322141](https://bugs.openjdk.org/browse/JDK-8322141) to track this. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427398726
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 20:21:10 GMT, Vladimir Sitnikov wrote: >>> What do you think? >> >> I think that you are looking at an obsolete repository: >> >> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802 > > I mean SequenceInputStream, not the base class: > https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249 > > Could you please double-check? IMHO @vlsi is right. It is incorrect that we stop the transfer in the overflow case. We should fix it as he suggested. I can do that if you like. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427342442
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 19:22:18 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/SequenceInputStream.java line 249:
>>
>>> 247: transferred = Math.addExact(transferred,
>>> in.transferTo(out));
>>> 248: } catch (ArithmeticException ignore) {
>>> 249: return Long.MAX_VALUE;
>>
>> @bplb , this looks like a bug to me: once `transferred` reaches
>> `Long.MAX_VALUE` the transfer loop will terminate and the transfer will stop
>> even in the case there are more streams available in the sequence.
>>
>> I think the proper code should be `transferred = Long.MAX_VALUE` instead of
>> `return Long.MAX_VALUE` (and there should be no `if (transferred <
>> Long.MAX_VALUE)` check)
>>
>> What do you think?
>
>> What do you think?
>
> I think that you are looking at an obsolete repository:
>
> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802
I mean SequenceInputStream, not the base class:
https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249
Could you please double-check?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427231492
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 06:12:40 GMT, Vladimir Sitnikov wrote: > What do you think? I think that you are looking at an obsolete repository: https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802 - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427171307
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Fri, 10 Feb 2023 17:38:51 GMT, Brian Burkhalter wrote:
>> `java.io.InputStream::transferTo` could conceivably return a negative value
>> if the count of bytes transferred overflows a `long`. Modify the method to
>> limit the number of bytes transferred to `Long.MAX_VALUE` per invocation.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8297632: Align SequenceInputStream style with other changes in patch
src/java.base/share/classes/java/io/SequenceInputStream.java line 249:
> 247: transferred = Math.addExact(transferred,
> in.transferTo(out));
> 248: } catch (ArithmeticException ignore) {
> 249: return Long.MAX_VALUE;
@bplb , this looks like a bug to me: once `transferred` reaches
`Long.MAX_VALUE` the transfer loop will terminate and the transfer will stop
even in the case there are more streams available in the sequence.
I think the proper code should be `transferred = Long.MAX_VALUE` instead of
`return Long.MAX_VALUE`.
What do you think?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1426217875
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Fri, 10 Feb 2023 17:38:51 GMT, Brian Burkhalter wrote: >> `java.io.InputStream::transferTo` could conceivably return a negative value >> if the count of bytes transferred overflows a `long`. Modify the method to >> limit the number of bytes transferred to `Long.MAX_VALUE` per invocation. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8297632: Align SequenceInputStream style with other changes in patch Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11403
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
> `java.io.InputStream::transferTo` could conceivably return a negative value > if the count of bytes transferred overflows a `long`. Modify the method to > limit the number of bytes transferred to `Long.MAX_VALUE` per invocation. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8297632: Align SequenceInputStream style with other changes in patch - Changes: - all: https://git.openjdk.org/jdk/pull/11403/files - new: https://git.openjdk.org/jdk/pull/11403/files/b1cd84ec..6bdf3220 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11403=06 - incr: https://webrevs.openjdk.org/?repo=jdk=11403=05-06 Stats: 8 lines in 1 file changed: 2 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11403.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11403/head:pull/11403 PR: https://git.openjdk.org/jdk/pull/11403
