8290353: ModuleReader::list specification should suggest closing the returned stream #9538

rjernst · 2022-07-18T14:06:00Z

This commit adds additional clarification to the javadocs of
ModuleReader::list about needing to close the stream. The new wording is
similar to that used in Files::find and other similar methods.

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
Change requires a CSR request to be approved

Issues

JDK-8290353: ModuleReader::list specification should suggest closing the returned stream
JDK-8290521: ModuleReader::list specification should suggest closing the returned stream (CSR)

Reviewers

Chris Hegarty (@ChrisHegarty - Reviewer) ⚠️ Review applies to 181c47a1
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 181c47a1
Mark Reinhold (@mbreinhold - Reviewer) ⚠️ Review applies to 181c47a1
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9538

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

Using diff file

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

This commit adds additional clarification to the javadocs of ModuleReader::list about needing to close the stream. The new wording is similar to that used in Files::find and other similar methods.

bridgekeeper · 2022-07-18T14:07:50Z

👋 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-18T14:10:48Z

@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-18T14:14:05Z

Webrevs

ChrisHegarty · 2022-07-18T14:53:19Z

src/java.base/share/classes/java/lang/module/ModuleReader.java

@@ -213,9 +213,17 @@ default void release(ByteBuffer bb) {
     * when using the stream to list the module contents and access is denied
     * by the security manager. </p>
     *
+     * <p> The returned stream contains references to one or more open directories
+     * in the module. The directories are closed by closing the stream. </p>


This mostly looks good - I agree with adding the note here. Should the wording be relaxed a little - replace "contains references" with "may contain references" - since the stream will not always contain references to open directories - it depends on the type of reader. The end result / guidance is the same - use in a try-with-resources block - I'm just suggesting a slight relaxing of the wording.

"may contain references" is good. I agree that the wordings should be relaxed a little. Similar for the apiNote.

ChrisHegarty · 2022-07-18T14:54:19Z

src/java.base/share/classes/java/lang/module/ModuleReader.java

     * <p> The behavior of the stream when used after the module reader is
     * closed is implementation specific and therefore not specified. </p>
     *
+     * @apiNote
+     * This method must be used within a try-with-resources statement or similar
+     * control structure to ensure that the stream's open directories are closed


Similar to my other comment - should "must' be replaced with "should" - for the same aforementioned reason.

ChrisHegarty · 2022-07-18T14:59:24Z

Given the recommendation to close streams returned by the list method, then this PR could be extended to update any usages in, at least, java.base. If there are many usages beyond java.base, then a separate issue could be filed to update them too. ( if not already covered by a previous PR in this area )

rjernst · 2022-07-18T18:15:22Z

I relaxed the wording in 0b3ca18 and addressed uses of list() in java.base in a72a5b9.

mbreinhold · 2022-07-18T19:48:02Z

It’s odd to mix adding some advice to the Javadoc with adopting that advice in the code base. Consider opening a new issue and PR for the actual code changes.

Also, a better summary for this issue (and PR) would be “ModuleReader::list specification should suggest closing the returned stream,” since that’s the essence of it.

ChrisHegarty · 2022-07-19T09:51:34Z

It’s odd to mix adding some advice to the Javadoc with adopting that advice in the code base. Consider opening a new issue and PR for the actual code changes.

JDK-8290504 has been filed to track the implementation changes. @rjernst please separate out the implementation changes into a separate PR, using 8290504 as the linked JIRA issue. (apologies, I previously asked for the implementation changes to be in this PR )

Also, a better summary for this issue (and PR) would be “ModuleReader::list specification should suggest closing the returned stream,” since that’s the essence of it.

Agreed. I updated the JIRA issue summary as suggested. @rjernst please update the title of this PR to match.

ChrisHegarty · 2022-07-19T10:39:53Z

/csr

ChrisHegarty · 2022-07-19T10:41:13Z

The following CSR has been filed for this issue, https://bugs.openjdk.org/browse/JDK-8290521. Please review.

openjdk · 2022-07-19T10:42:02Z

@ChrisHegarty this pull request will not be integrated until the CSR request JDK-8290521 for issue JDK-8290353 has been approved.

This reverts commit a72a5b9.

ChrisHegarty

LGTM

mlchung · 2022-07-19T16:13:59Z

Looks good to me. I reviewed the CSR.

mbreinhold

Ship it!

openjdk · 2022-07-20T00:39:40Z

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

8290353: ModuleReader::list specification should suggest closing the returned stream

Reviewed-by: chegar, mchung, mr, jpai

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

e8975be: 8290746: ProblemList compiler/vectorization/TestAutoVecIntMinMax.java
9c19d89: Merge
17e65bb: 8290625: Test jdk/javadoc/tool/CheckManPageOptions.java after manpage update
618f3a8: 8278274: Update nroff pages in JDK 19 before RC
f1001a0: 8287916: Address the inconsistency between the constant array and pool size
5d1c448: 8285407: Improve Xalan supports
d0a2f13: 8282676: Improve subject handling
8333950: 8284370: Improve zlib usage
879ea78: 8283190: Improve MIDI processing
78c514d: 8281866: Enhance MethodHandle invocations
... and 4 more: https://git.openjdk.org/jdk/compare/b1817a30a00d74f70b247c783047bfbb49515dda...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 (@ChrisHegarty, @mlchung, @mbreinhold, @jaikiran) 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 · 2022-07-20T03:26:12Z

Hello Ryan, the change looks fine to me. Please update the copyright year on the ModuleReader.java file, from 2020 to 2022.

rjernst · 2022-07-20T20:53:57Z

/integrate

openjdk · 2022-07-20T20:57:07Z

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

jaikiran

Thank you Ryan for the copyright change. Looks fine to me.

ChrisHegarty · 2022-07-21T06:14:44Z

/sponsor

openjdk · 2022-07-21T06:16:16Z

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

2c73a1f: 8290324: Move atomic operations outside of os_xxx.hpp
e8975be: 8290746: ProblemList compiler/vectorization/TestAutoVecIntMinMax.java
9c19d89: Merge
17e65bb: 8290625: Test jdk/javadoc/tool/CheckManPageOptions.java after manpage update
618f3a8: 8278274: Update nroff pages in JDK 19 before RC
f1001a0: 8287916: Address the inconsistency between the constant array and pool size
5d1c448: 8285407: Improve Xalan supports
d0a2f13: 8282676: Improve subject handling
8333950: 8284370: Improve zlib usage
879ea78: 8283190: Improve MIDI processing
... and 5 more: https://git.openjdk.org/jdk/compare/b1817a30a00d74f70b247c783047bfbb49515dda...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-21T06:16:41Z

@ChrisHegarty @rjernst Pushed as commit db1e44c.

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

8290353: Clarify the streams returned by ModuleReader::list

6c9750f

This commit adds additional clarification to the javadocs of ModuleReader::list about needing to close the stream. The new wording is similar to that used in Files::find and other similar methods.

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

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

ChrisHegarty reviewed Jul 18, 2022

View reviewed changes

rjernst added 2 commits July 18, 2022 11:11

fix uses of list() in java.base

a72a5b9

adjust wording

0b3ca18

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 19, 2022

rjernst added 2 commits July 19, 2022 07:00

Revert "fix uses of list() in java.base"

bc6192d

This reverts commit a72a5b9.

Merge branch 'master' into try_files/module_reader

181c47a

rjernst changed the title ~~8290353: Clarify the streams returned by ModuleReader::list~~ 8290353: ModuleReader::list specification should suggest closing the returned stream Jul 19, 2022

ChrisHegarty approved these changes Jul 19, 2022

View reviewed changes

mlchung approved these changes Jul 19, 2022

View reviewed changes

mbreinhold approved these changes Jul 19, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Jul 20, 2022

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

update copyright

75194c5

Merge branch 'master' into try_files/module_reader

58ac453

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

jaikiran approved these changes Jul 21, 2022

View reviewed changes

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

8290353: ModuleReader::list specification should suggest closing the returned stream #9538

8290353: ModuleReader::list specification should suggest closing the returned stream #9538

rjernst commented Jul 18, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

mlbridge bot commented Jul 18, 2022 •

edited

Loading

ChrisHegarty Jul 18, 2022

mlchung Jul 18, 2022

ChrisHegarty Jul 18, 2022

ChrisHegarty commented Jul 18, 2022 •

edited

Loading

rjernst commented Jul 18, 2022

mbreinhold commented Jul 18, 2022

ChrisHegarty commented Jul 19, 2022

ChrisHegarty commented Jul 19, 2022

ChrisHegarty commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

ChrisHegarty left a comment

mlchung commented Jul 19, 2022

mbreinhold left a comment

openjdk bot commented Jul 20, 2022 •

edited

Loading

jaikiran commented Jul 20, 2022

rjernst commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

jaikiran left a comment

ChrisHegarty commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

8290353: ModuleReader::list specification should suggest closing the returned stream #9538

8290353: ModuleReader::list specification should suggest closing the returned stream #9538

Conversation

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

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

mlbridge bot commented Jul 18, 2022 • edited Loading

Webrevs

ChrisHegarty Jul 18, 2022

Choose a reason for hiding this comment

mlchung Jul 18, 2022

Choose a reason for hiding this comment

ChrisHegarty Jul 18, 2022

Choose a reason for hiding this comment

ChrisHegarty commented Jul 18, 2022 • edited Loading

rjernst commented Jul 18, 2022

mbreinhold commented Jul 18, 2022

ChrisHegarty commented Jul 19, 2022

ChrisHegarty commented Jul 19, 2022

ChrisHegarty commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

ChrisHegarty left a comment

Choose a reason for hiding this comment

mlchung commented Jul 19, 2022

mbreinhold left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 20, 2022 • edited Loading

jaikiran commented Jul 20, 2022

rjernst commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

jaikiran left a comment

Choose a reason for hiding this comment

ChrisHegarty commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

rjernst commented Jul 18, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 18, 2022 •

edited

Loading

ChrisHegarty commented Jul 18, 2022 •

edited

Loading

openjdk bot commented Jul 20, 2022 •

edited

Loading