8279283 - BufferedInputStream should override transferTo #6935

mkarg · 2021-12-26T17:30:41Z

Implementation of JDK-8279283

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8279283: BufferedInputStream should override transferTo

Reviewers

@stsypanov (no known github.com user name / role) ⚠️ Review applies to b4d5004d
Brian Burkhalter (@bplb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/6935/head:pull/6935
$ git checkout pull/6935

Update a local copy of the PR:
$ git checkout pull/6935
$ git pull https://git.openjdk.org/jdk pull/6935/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6935

View PR using the GUI difftool:
$ git pr show -t 6935

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/6935.diff

bridgekeeper · 2021-12-26T17:32:19Z

👋 Welcome back mkarg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-12-26T17:34:41Z

@mkarg The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-12-26T17:38:39Z

Webrevs

AlanBateman · 2021-12-27T08:50:26Z

BIS is not specified to be thread safe but the existing read/write operations are. If transferTo is overridden then this is an area that will require close attention.

Have you surveyed the existing tests to see if transferTo is invoked on a BIS? New tests may be needed.

mkarg · 2021-12-27T09:30:25Z

BIS is not specified to be thread safe but the existing read/write operations are. If transferTo is overridden then this is an area that will require close attention.

In analogy to the other read/write operations I now have synchronized transferTo in 8dac240 to be on the safe side.

Have you surveyed the existing tests to see if transferTo is invoked on a BIS? New tests may be needed.

Did not find an existing test for BIS.transferTo, so I will write a new test for this.

mkarg · 2021-12-27T13:03:39Z

Have you surveyed the existing tests to see if transferTo is invoked on a BIS? New tests may be needed.

I have provided a test for BIS.transferTo in fbc5def.

stsypanov · 2022-01-03T19:55:58Z

Maybe we need to include into this patch the benchmark referenced to in the body of the ticket?

AlanBateman · 2022-01-06T13:20:12Z

I think you'll need to look at the interaction with mark/reset. I don't think you can bypass the buffering when there is a mark set.

src/java.base/share/classes/java/io/BufferedInputStream.java

mkarg · 2022-01-15T09:31:51Z

Good catches, I will look into your comments!

bridgekeeper · 2022-02-12T13:15:44Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-02-12T14:15:34Z

Please keep open, still working on it.

bridgekeeper · 2022-03-12T19:47:10Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-03-13T09:04:48Z

Please keep this PR open; I will continune work on it soon.

bridgekeeper · 2022-04-10T14:04:10Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-04-10T14:30:37Z

Please keep this PR open; I will continune work on it soon.

bridgekeeper · 2022-05-08T17:34:22Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-05-13T17:38:57Z

Please keep this PR open; I will continune work on it soon.

AlanBateman · 2022-06-06T07:15:51Z

Can this PR be closed or returned to daft?

mkarg · 2022-06-06T14:49:51Z

I think we should turn it back to draft.

bridgekeeper · 2022-07-04T22:32:12Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg · 2022-07-06T17:02:41Z

Please keep this PR open; I will continune work on it soon.

src/java.base/share/classes/java/io/BufferedInputStream.java

bridgekeeper · 2022-08-04T15:21:15Z

@mkarg This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-09-02T06:43:36Z

@mkarg This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

stsypanov · 2022-09-02T07:34:17Z

Will this be reopened somewhen? The proposed changes seems useful

test/jdk/java/io/BufferedInputStream/TransferTo.java

mkarg · 2022-09-24T12:14:49Z

@bplb Fixed the issues you pointed out. Kindly requesting approval. :-)

AlanBateman · 2022-09-25T07:28:56Z

@AlanBateman Kindly requesting approval. :-)

I don't have any more comments, the recent test cleanups are good. I assume @bplb will sponsor.

mkarg · 2022-09-25T07:58:37Z

@AlanBateman Kindly requesting approval. :-)

I don't have any more comments, the recent test cleanups are good. I assume @bplb will sponsor.

Thanks a lot for all your kind help with this PR! :-)

mkarg · 2022-09-25T10:18:50Z

@AlanBateman Kindly asking for /approve. Thanks. :-)

bplb · 2022-09-26T21:35:26Z

@bplb Fixed the issues you pointed out.

@mkarg You are welcome.

openjdk · 2022-09-26T21:39:27Z

⚠️ @mkarg the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout bufferedinputstream-transferto
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2022-09-26T21:39:32Z

@mkarg This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8279283: BufferedInputStream should override transferTo

Reviewed-by: bpb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 263 new commits pushed to the master branch:

79ccc79: 8293613: need to properly handle and hide tmp VTMS transitions
5e1e449: 8290920: sspi_bridge.dll not built if BUILD_CRYPTO is false
d827fd8: 8294430: RISC-V: Small refactoring for movptr_with_offset
9d76ac8: 8292158: AES-CTR cipher state corruption with AVX-512
e5b65c4: 8290482: Update JNI Specification of DestroyJavaVM for better alignment with JLS, JVMS, and Java SE API Specifications
f8d9fa8: 8294483: Remove vmTestbase/nsk/jvmti/GetThreadState tests.
6ad151d: 8293143: Workaround for JDK-8292217 when doing "step over" of bytecode with unresolved cp reference
22b59b6: 8294471: SpecTaglet is inconsistent with SpecTree for inline property
763d4bf: 8293592: Remove JVM_StopThread, stillborn, and related cleanup
739fdec: 8289162: runtime/NMT/ThreadedMallocTestType.java should print out memory allocations to help debug
... and 253 more: https://git.openjdk.org/jdk/compare/ef20ffe4d222d48f0bdba81a0b864d9fb455e9a6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bplb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mkarg · 2022-09-28T05:50:13Z

/integrate

openjdk · 2022-09-28T05:52:52Z

@mkarg
Your change (at version c30ea5d) is now ready to be sponsored by a Committer.

mkarg · 2022-09-28T06:25:07Z

@bplb I would be very glad if you would /sponsor my PR. :-)

bplb · 2022-09-28T15:55:36Z

/sponsor

bplb · 2022-09-28T15:55:52Z

@bplb I would be very glad if you would /sponsor my PR. :-)

Done.

openjdk · 2022-09-28T15:58:41Z

Going to push as commit 7515b30.
Since your change was applied there have been 273 commits pushed to the master branch:

7401fe0: 8292912: Make guard card in CardTable inaccessible
70d8428: 8294520: Problemlist java/nio/file/Files/CopyProcFile.java
30e3bf9: 8291805: IGV: Improve Zooming
37f83b9: 8294375: test/jdk/java/nio/channels/vthread/BlockingChannelOps.java is slow
60616f2: 8294059: Serial: Refactor GenCollectedHeap::collect
ea61671: 8294359: Interpreter(AArch64) intrinsify Thread.currentThread()
c42ef70: 7148092: [macosx] When Alt+down arrow key is pressed, the combobox popup does not appear.
94e14da: 8294057: Parallel: Tighten ParallelCompactData::initialize_region_data
1ea0d6b: 8292301: [REDO v2] C2 crash when allocating array of size too large
c13e0ef: 8292848: AWT_Mixing and TrayIcon tests fail on el8 with hard-coded isOel7
... and 263 more: https://git.openjdk.org/jdk/compare/ef20ffe4d222d48f0bdba81a0b864d9fb455e9a6...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-28T15:59:00Z

@bplb @mkarg Pushed as commit 7515b30.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mkarg · 2022-10-01T20:35:13Z

I think you are asking if is safe to leak a reference to the internal buffer. If there is no mark then it might be okay because there is no replay for an evil output stream to attack. However, I think it would require wider review to be confident that there aren't other interesting ways to break it; hence the suggestion in one of the earlier comments to keep it simple and limit it when there is no subclassing, no mark, and no bytes buffered. This does not prevent widening the conditions in the future.

@AlanBateman I opened another PR to continue this discussion.

openjdk bot added the rfr Pull request is ready for review label Dec 26, 2021

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 26, 2021

mkarg changed the title ~~8279283 - (ch) BufferedInputStream should override transferTo~~ 8279283 - BufferedInputStream should override transferTo Dec 27, 2021

dfuch reviewed Jan 12, 2022

View reviewed changes

src/java.base/share/classes/java/io/BufferedInputStream.java Show resolved Hide resolved

stsypanov reviewed Jul 7, 2022

View reviewed changes

src/java.base/share/classes/java/io/BufferedInputStream.java Outdated Show resolved Hide resolved

bridgekeeper bot closed this Sep 2, 2022

bplb reviewed Sep 22, 2022

View reviewed changes

test/jdk/java/io/BufferedInputStream/TransferTo.java Outdated Show resolved Hide resolved

bplb reviewed Sep 22, 2022

View reviewed changes

test/jdk/java/io/BufferedInputStream/TransferTo.java Outdated Show resolved Hide resolved

bplb reviewed Sep 22, 2022

View reviewed changes

test/jdk/java/io/BufferedInputStream/TransferTo.java Outdated Show resolved Hide resolved

mkarg added 3 commits September 24, 2022 12:52

removed unused import

a2ca2c0

wrapped long code lines

48b1f29

transformTo -> transferTo

c30ea5d

bplb approved these changes Sep 26, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 26, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 28, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 28, 2022

openjdk bot closed this Sep 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 28, 2022

mkarg deleted the bufferedinputstream-transferto branch September 28, 2022 16:11

8279283 - BufferedInputStream should override transferTo #6935

8279283 - BufferedInputStream should override transferTo #6935

Conversation

mkarg commented Dec 26, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 26, 2021

openjdk bot commented Dec 26, 2021

mlbridge bot commented Dec 26, 2021 • edited Loading

Webrevs

AlanBateman commented Dec 27, 2021

mkarg commented Dec 27, 2021

mkarg commented Dec 27, 2021

stsypanov commented Jan 3, 2022

AlanBateman commented Jan 6, 2022

mkarg commented Jan 15, 2022

bridgekeeper bot commented Feb 12, 2022

mkarg commented Feb 12, 2022

bridgekeeper bot commented Mar 12, 2022

mkarg commented Mar 13, 2022

bridgekeeper bot commented Apr 10, 2022

mkarg commented Apr 10, 2022

bridgekeeper bot commented May 8, 2022

mkarg commented May 13, 2022

AlanBateman commented Jun 6, 2022

mkarg commented Jun 6, 2022

bridgekeeper bot commented Jul 4, 2022

mkarg commented Jul 6, 2022

bridgekeeper bot commented Aug 4, 2022

bridgekeeper bot commented Sep 2, 2022

stsypanov commented Sep 2, 2022

mkarg commented Sep 24, 2022

AlanBateman commented Sep 25, 2022

mkarg commented Sep 25, 2022

mkarg commented Sep 25, 2022

bplb commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

openjdk bot commented Sep 26, 2022 • edited Loading

mkarg commented Sep 28, 2022

openjdk bot commented Sep 28, 2022

mkarg commented Sep 28, 2022

bplb commented Sep 28, 2022

bplb commented Sep 28, 2022

openjdk bot commented Sep 28, 2022

openjdk bot commented Sep 28, 2022

mkarg commented Oct 1, 2022

mkarg commented Dec 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 26, 2021 •

edited

Loading

openjdk bot commented Sep 26, 2022 •

edited

Loading