8290504: Close streams returned by ModuleReader::list #9557

rjernst · 2022-07-19T14:07:17Z

This commit ensures streams returned by ModuleReader::list are closed.

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-8290504: Close streams returned by ModuleReader::list

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 3fc8cca8
Chris Hegarty (@ChrisHegarty - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9557

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

Using diff file

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

This commit ensures streams returned by ModuleReader::list are closed.

bridgekeeper · 2022-07-19T14:08:36Z

👋 Welcome back rjernst! 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 · 2022-07-19T14:12:19Z

@rjernst 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 · 2022-07-19T14:16:18Z

Webrevs

rjernst · 2022-07-19T14:16:30Z

Note that ModulePatcher::list delegates to ModuleReader::list, but since the stream it builds closes the underlying stream, the use doesn't need to be closed within the list method.

mlchung

LGTM

openjdk · 2022-07-20T00:42:02Z

@rjernst 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:

8290504: Close streams returned by ModuleReader::list

Reviewed-by: mchung, chegar

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 5 new commits pushed to the master branch:

15f4b30: 8290115: ArrayCopyObject JMH has wrong package
4c1cd66: 8288368: simplify code in ValueTaglet, remove redundant code
6346c33: 8290826: validate-source failures after JDK-8290016
604a115: 8290016: IGV: Fix graph panning when mouse dragged outside of window
59e495e: 8290704: x86: TemplateTable::_new should not call eden_allocate() without contiguous allocs enabled

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@mlchung, @jaikiran, @ChrisHegarty) 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).

jaikiran

Hello Ryan, the failures reported in the GitHub Actions jobs look related to this change. One such failure is in CallerSensitiveAccess test which throws an exception as follows:

java.lang.RuntimeException: java.lang.IllegalStateException: source already consumed or closed
	at org.testng.internal.MethodInvocationHelper.invokeMethodNoCheckedException(MethodInvocationHelper.java:49)
	at org.testng.internal.MethodInvocationHelper.invokeDataProvider(MethodInvocationHelper.java:145)
	at org.testng.internal.Parameters.handleParameters(Parameters.java:797)
	at org.testng.internal.Parameters.handleParameters(Parameters.java:740)
	at org.testng.internal.ParameterHandler.handleParameters(ParameterHandler.java:59)
	at org.testng.internal.ParameterHandler.createParameters(ParameterHandler.java:38)
	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:789)
	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.testng.TestRunner.privateRun(TestRunner.java:764)
	at org.testng.TestRunner.run(TestRunner.java:585)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
	at org.testng.SuiteRunner.run(SuiteRunner.java:286)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
	at org.testng.TestNG.runSuites(TestNG.java:1069)
	at org.testng.TestNG.run(TestNG.java:1037)
	at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94)
	at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
	at java.base/java.lang.Thread.run(Thread.java:1589)
Caused by: java.lang.IllegalStateException: source already consumed or closed
	at java.base/java.util.stream.AbstractPipeline.sourceSpliterator(AbstractPipeline.java:409)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at CallerSensitiveAccess.callerSensitiveMethods(CallerSensitiveAccess.java:67)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:76)
	at org.testng.internal.MethodInvocationHelper.invokeMethodNoCheckedException(MethodInvocationHelper.java:45)
	... 28 more

ChrisHegarty · 2022-07-20T08:14:39Z

test/jdk/java/lang/invoke/callerSensitive/CallerSensitiveAccess.java

-            return reader.list()
+        try (ModuleReader reader = mref.open();
+             Stream<String> stream = reader.list()) {
+            return stream


This change is causing the test to fail, in the callerSensitiveMethods DataProvider, because the data provider is expecting an open stream to be returned by callerSensitiveMethods(Module) - the stream is now closed.

There are a couple of ways to resolve this, but the most straightforward would be to revert this part of the change, and have the callerSensitiveMethods DataProvider close the returned stream. E.g.:

@DataProvider(name = "callerSensitiveMethods") static Object[][] callerSensitiveMethods() { try (var methodStream = callerSensitiveMethods(Object.class.getModule())) { return methodStream .map(m -> new Object[]{m, shortDescription(m)}) .toArray(Object[][]::new); } }

yes, this would solve it.

Done in 4a94c64

@rjernst it seems that the revert part of the above has been done, but not the "have the callerSensitiveMethods DataProvider close the returned stream".

Sorry I misunderstood. Fixed now in 3080378.

JarvisCraft · 2022-07-20T21:11:04Z

src/java.base/share/classes/jdk/internal/module/ModuleHashes.java

@@ -114,9 +115,9 @@ private static byte[] computeHash(ModuleReader reader, String algorithm) {
        } catch (NoSuchAlgorithmException e) {
            throw new IllegalArgumentException(e);
        }
-        try {
+        try (Stream<String> stream = reader.list()){


Suggested change

try (Stream<String> stream = reader.list()){

try (Stream<String> stream = reader.list()) {

openjdk · 2022-07-21T13:18:45Z

@rjernst this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout try_files/module_reader_uses
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

ChrisHegarty

LGTM.

rjernst · 2022-07-21T17:51:13Z

/integrate

openjdk · 2022-07-21T17:54:06Z

@rjernst
Your change (at version 5d09188) is now ready to be sponsored by a Committer.

ChrisHegarty · 2022-07-21T17:59:31Z

/sponsor

openjdk · 2022-07-21T18:01:38Z

Going to push as commit 80bd8c3.
Since your change was applied there have been 5 commits pushed to the master branch:

15f4b30: 8290115: ArrayCopyObject JMH has wrong package
4c1cd66: 8288368: simplify code in ValueTaglet, remove redundant code
6346c33: 8290826: validate-source failures after JDK-8290016
604a115: 8290016: IGV: Fix graph panning when mouse dragged outside of window
59e495e: 8290704: x86: TemplateTable::_new should not call eden_allocate() without contiguous allocs enabled

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-21T18:01:56Z

@ChrisHegarty @rjernst Pushed as commit 80bd8c3.

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

8290504: Close streams returned by ModuleReader::list

3fc8cca

This commit ensures streams returned by ModuleReader::list are closed.

openjdk bot added the rfr Pull request is ready for review label Jul 19, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 19, 2022

mlchung approved these changes Jul 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 20, 2022

jaikiran suggested changes Jul 20, 2022

View reviewed changes

ChrisHegarty reviewed Jul 20, 2022

View reviewed changes

rjernst added 2 commits July 20, 2022 13:50

revert CallerSensitiveAccess change

4a94c64

Merge branch 'master' into try_files/module_reader_uses

d09846f

JarvisCraft suggested changes Jul 20, 2022

View reviewed changes

rjernst added 3 commits July 20, 2022 14:54

silly spaces

b041233

fix caller sensitive test uses

3080378

Merge branch 'master' into try_files/module_reader_uses

5d09188

openjdk bot added merge-conflict Pull request has merge conflict with target branch ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated merge-conflict Pull request has merge conflict with target branch labels Jul 21, 2022

ChrisHegarty approved these changes Jul 21, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 21, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 21, 2022

openjdk bot closed this Jul 21, 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 Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290504: Close streams returned by ModuleReader::list #9557

8290504: Close streams returned by ModuleReader::list #9557

rjernst commented Jul 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

mlbridge bot commented Jul 19, 2022 •

edited

Loading

rjernst commented Jul 19, 2022

mlchung left a comment

openjdk bot commented Jul 20, 2022 •

edited

Loading

jaikiran left a comment

ChrisHegarty Jul 20, 2022

mlchung Jul 20, 2022

rjernst Jul 20, 2022

ChrisHegarty Jul 21, 2022

rjernst Jul 21, 2022

JarvisCraft Jul 20, 2022

openjdk bot commented Jul 21, 2022

ChrisHegarty left a comment

rjernst commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

ChrisHegarty commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

	try (Stream<String> stream = reader.list()){
	try (Stream<String> stream = reader.list()) {

8290504: Close streams returned by ModuleReader::list #9557

8290504: Close streams returned by ModuleReader::list #9557

Conversation

rjernst commented Jul 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

mlbridge bot commented Jul 19, 2022 • edited Loading

Webrevs

rjernst commented Jul 19, 2022

mlchung left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 20, 2022 • edited Loading

jaikiran left a comment

Choose a reason for hiding this comment

ChrisHegarty Jul 20, 2022

Choose a reason for hiding this comment

mlchung Jul 20, 2022

Choose a reason for hiding this comment

rjernst Jul 20, 2022

Choose a reason for hiding this comment

ChrisHegarty Jul 21, 2022

Choose a reason for hiding this comment

rjernst Jul 21, 2022

Choose a reason for hiding this comment

JarvisCraft Jul 20, 2022

Choose a reason for hiding this comment

openjdk bot commented Jul 21, 2022

ChrisHegarty left a comment

Choose a reason for hiding this comment

rjernst commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

ChrisHegarty commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

rjernst commented Jul 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 19, 2022 •

edited

Loading

openjdk bot commented Jul 20, 2022 •

edited

Loading