Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 22:46:32 GMT, Markus KARG wrote: > So I did not do something wrong but finally I learned Github forensics now. If no crime was committed, I would call it archaeology. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1865249242
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 18:49:39 GMT, Brian Burkhalter wrote: > > this icon only says that Alan made change requests > > I think it's if the "changes requested" button is selected in the "Submit > Review" popup under "Files Changed." Typically yes. But not in the current case: Alan pressed "approve" first *without* giving a comment leading to this entry:  ...then...  ...and **after that** Alan pressed `Request changes` button in the review dialog (note the heading "Alan Bateman suggested changes last week"):  So the comment "I think jai is right" was the trigger to set that icon, but that icon was definitively **not** set before. So I did not do something wrong but finally I learned Github forensics now. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1865242504
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 18:28:27 GMT, Markus KARG wrote: > this icon only says that Alan made change requests I think it's if the "changes requested" button is selected in the "Submit Review" popup under "Files Changed." - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864976239
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 17:57:57 GMT, Brian Burkhalter wrote: > > But where do you see a change request from Alan _before_ I posted > > `/integrate`? > > Mouse-over the icon to the right of his name at the top right under > "Reviewers." I might be wrong, but IMHO this icon only says that Alan made change requests, but not *when*. This means, the icon pops up even when requesting the changes *after* approving. But I again, I might be wrong. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864945556
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long > But where do you see a change request from Alan _before_ I posted > `/integrate`? Mouse-over the icon to the right of his name at the top right under "Reviewers." > BTW, here is the "Ready" label you did not see: [#17119 > (comment)](https://github.com/openjdk/jdk/pull/17119#event-11256913866) Ack. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864901444
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 16:52:38 GMT, Brian Burkhalter wrote: > > Alan actually did approve them on December 15. > > I know, with requested changes. I didn't see the request transition to > "Ready" which is where it needs to be to `/integrate`. Looking at the Github history I need to say that I cannot see that Alan approved *with change requests*. Maybe I am missing something. I see his approval on Dec 15 directly before my `/integrate` on the same day, but no change request *there*, and I can see his change request *after* my `/integrate` where he says "I think jai is right". But where do you see a change request from Alan *before* I posted `/integrate`? I am pretty sure you are right, but where actually do you see this in the Github history? Thanks. :-) BTW, here is the "Ready" label you did not see: https://github.com/openjdk/jdk/pull/17119#event-11256913866 - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864893934
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 00:56:16 GMT, Jaikiran Pai wrote: > The updated source change looks fine to me. @jaikiran Thanks for the corroboration. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864827299
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 16:52:38 GMT, Brian Burkhalter wrote: >>> Approved. >>> >>> Please note in future that it is better not to `/integrate` until the >>> request has actually been approved by a Reviewer. >> >> Alan actually did approve them on December 15. > >> Alan actually did approve them on December 15. > > I know, with requested changes. I didn't see the request transition to > "Ready" which is where it needs to be to `/integrate`. > @bplb Apparently Skara wants you to repeat your sponsoring again. Yes, because the PR was updated since the previous `/integrate`. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864817465
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 09:47:05 GMT, Markus KARG wrote: > Alan actually did approve them on December 15. I know, with requested changes. I didn't see the request transition to "Ready" which is where it needs to be to `/integrate`. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864815013
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Tue, 19 Dec 2023 18:31:19 GMT, Brian Burkhalter wrote: >> Markus KARG has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test was using Integer but must use Long > > I verified that the test now fails without the source change but passes with > it. @bplb Apparently Skara wants you to repeat your sponsoring again. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864199779
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Tue, 19 Dec 2023 18:20:05 GMT, Brian Burkhalter wrote: > Approved. > > Please note in future that it is better not to `/integrate` until the request > has actually been approved by a Reviewer. Alan actually did approve them on December 15. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864164861
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long Thank you Markus for the update. The updated source change looks fine to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17119#pullrequestreview-1789914334
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long I verified that the test now fails without the source change but passes with it. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1863284534
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long Approved. Please note in future that it is better not to `/integrate` until the request has actually been approved by a Reviewer. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17119#pullrequestreview-1789475686
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 19:10:49 GMT, Vladimir Sitnikov wrote: >> Markus KARG has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test was using Integer but must use Long > > test/jdk/java/io/SequenceInputStream/TransferTo.java line 143: > >> 141: SequenceInputStream sis = new SequenceInputStream(is1, is2); >> 142: OutputStream nos = OutputStream.nullOutputStream(); >> 143: sis.transferTo(nos); > > Suggestion: > > assertEquals(sis.transferTo(nos), Long.MAX_VALUE, ".transferTo should > return Long.MAX_VALUE when transferring more than that amount of bytes"); The intention is not to fully test the complete functionality of `transferTo` but solely to test what *this* PR is fixing. It was never broken that transferTo returned the correct value (that bug never existed), and there is already a test for transferTo contents across streams in place. - PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1429170643
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 19:09:05 GMT, Vladimir Sitnikov wrote: >> Markus KARG has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test was using Integer but must use Long > > test/jdk/java/io/SequenceInputStream/TransferTo.java line 139: > >> 137: InputStream is1 = repeat(0, Long.MAX_VALUE); >> 138: InputStream is2 = repeat(0, 1); >> 139: assertNotEquals(is1.available(), 0); > > Wdyt of asserting the expected value with assertEquals or removing the > assertions before .transferTo altogether? > Suggestion: > > assertEquals(is1.available(), Integer.MAX_VALUE, "The stream should > have Long.MAX_VALUE bytes .available() before .transferTo (.available returns > int)"); The reader shall understand what the test is good for and what it works like. There is no benefit in checking the *actual* value (there is no need to explicitly fail if `available` returns something *else*, as long as it is *not zero*), so this would confuse the reader more than it helps. I kept the before-assertations solely to proof that the before-value of `available()` is *not the default value* of `InputStream`, hence to proof that the source code of this test is actually able to detect a *change*. We can remove this, it is not needed, but it makes it easier *to understand* what this test actually expects *to change*. - PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1429169709
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 19:03:26 GMT, Vladimir Sitnikov
wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Test was using Integer but must use Long
>
> test/jdk/java/io/SequenceInputStream/TransferTo.java line 154:
>
>> 152: * but fails with any other operation.
>> 153: */
>> 154: private static InputStream repeat(int b, long count) {
>
> `b` parameter seems to be unused. Wdyt of removing it?
Kept it for future use: If in future we like to add `write(b)` in `transferTo`
then we do not have to rework the call sites.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1429168196
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long test/jdk/java/io/SequenceInputStream/TransferTo.java line 139: > 137: InputStream is1 = repeat(0, Long.MAX_VALUE); > 138: InputStream is2 = repeat(0, 1); > 139: assertNotEquals(is1.available(), 0); Wdyt of asserting the expected value with assertEquals or removing the assertions before .transferTo altogether? Suggestion: assertEquals(is1.available(), Integer.MAX_VALUE, "The stream should have Long.MAX_VALUE bytes .available() before .transferTo (.available returns int)"); test/jdk/java/io/SequenceInputStream/TransferTo.java line 143: > 141: SequenceInputStream sis = new SequenceInputStream(is1, is2); > 142: OutputStream nos = OutputStream.nullOutputStream(); > 143: sis.transferTo(nos); Suggestion: assertEquals(sis.transferTo(nos), Long.MAX_VALUE, ".transferTo should return Long.MAX_VALUE when transferring more than that amount of bytes"); - PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1428890685 PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1428891013
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote:
>> Fixes JDK-8322141
>>
>> As suggested by @vlsi and documented by @bplb this PR does not return, but
>> only sets the maximum result value.
>
> Markus KARG has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test was using Integer but must use Long
test/jdk/java/io/SequenceInputStream/TransferTo.java line 154:
> 152: * but fails with any other operation.
> 153: */
> 154: private static InputStream repeat(int b, long count) {
`b` parameter seems to be unused. Wdyt of removing it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17119#discussion_r1428889592
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long Kindly asking for review. :-) - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1858879606
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
> Fixes JDK-8322141 > > As suggested by @vlsi and documented by @bplb this PR does not return, but > only sets the maximum result value. Markus KARG has updated the pull request incrementally with one additional commit since the last revision: Test was using Integer but must use Long - Changes: - all: https://git.openjdk.org/jdk/pull/17119/files - new: https://git.openjdk.org/jdk/pull/17119/files/2aaac630..556a8dc4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17119=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17119=02-03 Stats: 18 lines in 1 file changed: 10 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17119.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17119/head:pull/17119 PR: https://git.openjdk.org/jdk/pull/17119
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Fri, 15 Dec 2023 22:40:17 GMT, Brian Burkhalter wrote: >>> > [...] maybe we need to come up with tests that quickly check the handling >>> > at this limit. >> >>> @mkarg Do you intend to provide any such test as part of this request? >> >> @bplb I quickly drafted an absolute minimal test for this in >> [2aaac63](https://github.com/openjdk/jdk/pull/17119/commits/2aaac630a3e6d1df0fd77f6b34c0f0d3a3b50292). >> IMHO this is sufficient for the current case. > >> I quickly drafted an absolute minimal test for this in [2aaac63] [...]. > > This test does not fail for me when run against my local build of the current > mainline. Usually, unless randomness is involved, a test should fail before > the proposed patch is applied but succeed thereafter. @bplb Test is fixed now. Sorry for the delay. I have updated the test (contained in https://github.com/openjdk/jdk/pull/17119/commits/556a8dc4d3d8ff157e8374625eb129a08926e0dd) and successfully tried it with the original code (test fails) and the fixed code (test succeeds). IMHO this should do the job. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1858826626
