Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAllBytes() #5824

Closed
wants to merge 4 commits into from

Conversation

fweimer-rh
Copy link

@fweimer-rh fweimer-rh commented Oct 5, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAllBytes()

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5824/head:pull/5824
$ git checkout pull/5824

Update a local copy of the PR:
$ git checkout pull/5824
$ git pull https://git.openjdk.java.net/jdk pull/5824/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5824

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5824.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 5, 2021

👋 Welcome back fweimer! 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 openjdk bot changed the title 8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAll… 8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAllBytes() Oct 5, 2021
@openjdk openjdk bot added the rfr label Oct 5, 2021
@fweimer-rh
Copy link
Author

@fweimer-rh fweimer-rh commented Oct 5, 2021

I had to increase the test timeout. Even the original test required about 8 minutes to run on my test machines, which has pretty good single-thread performance.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

@fweimer-rh The following label will be automatically applied to this pull request:

  • nio

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.

@openjdk openjdk bot added the nio label Oct 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 5, 2021

Webrevs

@openjdk openjdk bot removed the rfr label Oct 5, 2021
* @summary Verify ChannelInputStream methods readAllBytes and readNBytes
* @library ..
* @library /test/lib
* @build jdk.test.lib.RandomFactory
* @modules java.base/jdk.internal.util
* @run testng/othervm -Xmx8G ReadXBytes
* @run testng/othervm/timeout=3600 -Xmx8G ReadXBytes
Copy link
Contributor

@AlanBateman AlanBateman Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you running the tests with the make target or jtreg without -timeoutFactor?

Copy link
Author

@fweimer-rh fweimer-rh Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm invoking jtreg directly, following Running tests using jtreg. That page doesn't mention -timeoutFactor.

Copy link
Contributor

@AlanBateman AlanBateman Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm invoking jtreg directly, following Running tests using jtreg. That page doesn't mention -timeoutFactor.

Okay, I think RunTests.gmk sets it to 4 but it can be overridden.

Copy link
Author

@fweimer-rh fweimer-rh Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I lower the timeout to 900 then? With a factor of 4, that should be enough to run the test even on fairly slow machines.

InputStream cis = Channels.newInputStream(ch)) {
f.test(length, cis, fis);
} finally {
Files.delete(file);
Copy link
Contributor

@AlanBateman AlanBateman Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see this cleaned up a bit. The line break at L154 make it hard to read. Having 4 resources in the same try-with-resources is also hard to read. The comment "relationship is obscured" will confuse further maintainers.

Copy link
Author

@fweimer-rh fweimer-rh Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important is exception correctness in the test code? Is it acceptable to leak resources on VM errors?

Copy link
Contributor

@AlanBateman AlanBateman Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important is exception correctness in the test code? Is it acceptable to leak resources on VM errors?

The tests usually run in agentvm mode so if failing tests leak then it may have knock impact to tests that run later in the same VM. So if we can clean up then we should. Sometimes we have tests that can't reliably cleanup, they can run in othervm mode that is runs just the one test.

Copy link
Author

@fweimer-rh fweimer-rh Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant whether it's acceptable to use a construct like try (var ch = Channels.newInputStream(new FileInputStream(f))). The input stream could leak there, but only in case of VM errors.

@openjdk openjdk bot added the rfr label Oct 5, 2021
InputStream cis = Channels.newInputStream(fc)) {
f.test(length, cis, fis);
try {
for (boolean hideSeek : List.of(false, true)) {
Copy link
Contributor

@AlanBateman AlanBateman Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a bit clearer if you renamed this to "seekable"

@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2021

@fweimer-rh 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:

8274780: ChannelInputStream.readNBytes(int) incorrectly calls readAllBytes()

Reviewed-by: alanb, 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 31 new commits pushed to the master branch:

  • 9945f7a: 8274318: Replace 'for' cycles with iterator with enhanced-for in java.management
  • 754bc82: 8274525: Replace uses of StringBuffer with StringBuilder in java.xml
  • 4e7d7ca: 8273711: Remove redundant stream() call before forEach in jdk.jlink
  • f3cedbe: 8274464: Remove redundant stream() call before forEach in java.* modules
  • c10de35: 8262944: Improve exception message when automatic module lists provider class not in JAR file
  • b8af6a9: 8273917: Remove 'leaf' ranking for Mutex
  • c80a612: 8273381: Assert in PtrQueueBufferAllocatorTest.stress_free_list_allocator_vm
  • 9759fcb: 8274496: Use String.contains() instead of String.indexOf() in java.desktop
  • cdf8930: 8274625: Search field placeholder behavior
  • df125f6: 8273410: IR verification framework fails with "Should find method name in validIrRulesMap"
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/8609ea55acdcc203408f58f7bf96ea9228aef613...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 (@AlanBateman, @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).

@openjdk openjdk bot added the ready label Oct 6, 2021
@fweimer-rh
Copy link
Author

@fweimer-rh fweimer-rh commented Oct 6, 2021

/integrate

@openjdk openjdk bot added the sponsor label Oct 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2021

@fweimer-rh
Your change (at version 43ec568) is now ready to be sponsored by a Committer.

bplb
bplb approved these changes Oct 6, 2021
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Oct 7, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

Going to push as commit 5762ec2.
Since your change was applied there have been 36 commits pushed to the master branch:

  • 4e960fe: 8274497: Unnecessary Vector usage in AquaFileSystemModel
  • c833b4d: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
  • d57fb6f: 8274456: Remove jtreg tag manual=yesno java/awt/print/PrinterJob/PageDialogTest.java
  • 734d1fb: 8274211: Test man page that options are documented
  • 9561fea: 8273102: Delete deprecated for removal the empty finalize() in java.desktop module
  • 9945f7a: 8274318: Replace 'for' cycles with iterator with enhanced-for in java.management
  • 754bc82: 8274525: Replace uses of StringBuffer with StringBuilder in java.xml
  • 4e7d7ca: 8273711: Remove redundant stream() call before forEach in jdk.jlink
  • f3cedbe: 8274464: Remove redundant stream() call before forEach in java.* modules
  • c10de35: 8262944: Improve exception message when automatic module lists provider class not in JAR file
  • ... and 26 more: https://git.openjdk.java.net/jdk/compare/8609ea55acdcc203408f58f7bf96ea9228aef613...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 7, 2021
@openjdk openjdk bot removed the sponsor label Oct 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2021

@AlanBateman @fweimer-rh Pushed as commit 5762ec2.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
3 participants