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

8273401: Disable JarIndex support in URLClassPath #5524

Closed
wants to merge 3 commits into from

Conversation

shiyuexw
Copy link
Contributor

@shiyuexw shiyuexw commented Sep 15, 2021

There is a bug for URLClassPath.findResources with JarIndex.
Currently, there was agreement on dropping the support from the URLClassLoader implementation but it was suggested that it should be disabled for a release or two before the code is removed.
A system property can be used to re-enable JarIndex support in URLClassPath.

The PR includes:
Disable JarIndex support in URLClassPath by default.
Add system property jdk.net.URLClassPath.enableJarIndex to re-enable JarIndex support.


Progress

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

Issue

  • JDK-8273401: Disable JarIndex Support In URLClassPath

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5524

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 15, 2021

👋 Welcome back shiyuexw! 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 added the rfr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@shiyuexw 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.

@openjdk openjdk bot added the core-libs label Sep 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 15, 2021

Webrevs

@@ -109,6 +110,9 @@
// the check is not disabled).
p = props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries");
DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : false;

p = props.getProperty("jdk.net.URLClassPath.enableJarIndex");
ENABLE_JAR_INDEX = p != null ? p.equals("true") : false;
Copy link
Member

@dfuch dfuch Sep 15, 2021

Choose a reason for hiding this comment

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

Maybe this should use the same pattern than the other property above:

ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false;

Copy link
Member

@dfuch dfuch Sep 15, 2021

Choose a reason for hiding this comment

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

Another question is where to document this property. We will obviously need a CSR and releases notes. I also wonder if it should be promoted into a networking property - which would make it possible to document it in the net.properties file. @AlanBateman what do you think?

Copy link
Contributor

@AlanBateman AlanBateman Sep 15, 2021

Choose a reason for hiding this comment

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

Another question is where to document this property. We will obviously need a CSR and releases notes. I also wonder if it should be promoted into a networking property - which would make it possible to document it in the net.properties file. @AlanBateman what do you think?

jdk.net.URLClassLoader.enableJarIndex is meant to be a temporary property. We could document it in URLClassLoader as an implNote but I'm not sure that it's worth it. I re-worded the CSR to reflect the current proposal and Wang has submitted it. I noted in the CSR that a release note is planned.

Copy link
Contributor

@AlanBateman AlanBateman Sep 15, 2021

Choose a reason for hiding this comment

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

Maybe this should use the same pattern than the other property above:

ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false;

Yes, the intention is that -Djdk.net.URLClassLoader.enableJarIndex will re-enable the feature.

Copy link
Contributor

@LanceAndersen LanceAndersen Sep 15, 2021

Choose a reason for hiding this comment

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

I assume the reason for specifically checking for a value of true or an empty string as you envision allowing the property to be explicitly set to false in the future?

Otherwise we could just check if the property is set(regardless of value) to re-enable the feature.

Copy link
Member

@dfuch dfuch Sep 15, 2021

Choose a reason for hiding this comment

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

I would dislike it if -Djdk.net.URLClassPath.enableJarIndex=false meant true...

Copy link
Contributor

@LanceAndersen LanceAndersen Sep 15, 2021

Choose a reason for hiding this comment

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

I can appreciate that and that is a fair point. I guess where I was coming from was that the property is temporary and has to be explicitly specified so ignoring the value seemed worth raising/discussing.

Assuming the current check is kept(which we should based on your input), it should probably be changed to ignore case.

Copy link
Member

@dfuch dfuch Sep 16, 2021

Choose a reason for hiding this comment

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

Given that there is a precedence in this file:

        p = props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries");
        DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : false;

I would tend to let the new property follow the same pattern, especially since it's temporary.

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 16, 2021

Choose a reason for hiding this comment

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

Yes,I changed the code to follow the same pattern.

Copy link
Contributor

@LanceAndersen LanceAndersen Sep 16, 2021

Choose a reason for hiding this comment

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

Given that there is a precedence in this file:

        p = props.getProperty("jdk.net.URLClassPath.showIgnoredClassPathEntries");
        DEBUG_CP_URL_CHECK = p != null ? p.equals("true") || p.isEmpty() : false;

I would tend to let the new property follow the same pattern, especially since it's temporary.

fair enough :-)

@dfuch
Copy link
Member

@dfuch dfuch commented Sep 15, 2021

/csr needed

@openjdk openjdk bot added the csr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@dfuch this pull request will not be integrated until the CSR request JDK-8273473 for issue JDK-8273401 has been approved.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 15, 2021

Thanks @shiyuexw for going through the iterations, I think we have a good proposal now.

@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 16, 2021

I have changed

ENABLE_JAR_INDEX = p != null ? p.equals("true") : false;

to

ENABLE_JAR_INDEX = p != null ? p.equals("true") || p.isEmpty() : false;

Furthemore, in order to maintain consistency in URLClassPath, the property name is "jdk.net.URLClassPath.enableJarIndex" .

dfuch
dfuch approved these changes Sep 16, 2021
dfuch
dfuch approved these changes Sep 16, 2021
@@ -940,7 +946,7 @@ Resource getResource(final String name, boolean check) {
if (entry != null)
return checkResource(name, check, entry);

if (index == null)
if (index == null || !ENABLE_JAR_INDEX)
Copy link
Contributor

@AlanBateman AlanBateman Sep 16, 2021

Choose a reason for hiding this comment

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

Is this needed? When ENABLE_JAR_INDEX is false then I assume index will always be null. I'm only asking is that it would be nice if ENABLE_JAR_INDEX was checked in one place rather than two.

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 16, 2021

Choose a reason for hiding this comment

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

Yes, the check is redundant, and I removed it.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Thanks, I think we can finalize the CSR now.

Copy link
Member

@mlchung mlchung left a comment

Looks good to me. I agree that this temporary property is not needed to be document in the javadoc.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 23, 2021

@shiyuexw Just a reminder that you need to finalize the CSR.

@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 24, 2021

@shiyuexw Just a reminder that you need to finalize the CSR.

Thanks a lot. We have finalized the CSR.

@openjdk openjdk bot removed the csr label Sep 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2021

⚠️ @shiyuexw 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 disablejarindex
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2021

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

8273401: Disable JarIndex support in URLClassPath

Reviewed-by: dfuchs, lancea, alanb, mchung

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

  • 5756385: 8274273: Update testing docs for MacOS with Non-US locale
  • 61ac53f: 8210927: JDB tests do not update source path after doing a redefine class
  • 341de49: 8273492: Move evacuation failure handling into G1YoungCollector
  • 13e9ea9: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125
  • 753b256: 8274296: Update or Problem List tests which may fail with uiScale=2 on macOS
  • baafa60: 8274237: Replace 'for' cycles with iterator with enhanced-for in java.base
  • 0c050be: 8274294: ProblemList sun/tools/jmap/BasicJMapTest.java
  • e741a18: 8274233: Minor cleanup for ToolBox
  • 718eff2: 8273380: ARM32: Default to {ldrexd,strexd} in StubRoutines::atomic_{load|store}_long
  • f214d6e: 8274234: Remove unnecessary boxing via primitive wrapper valueOf(String) methods in java.sql.rowset
  • ... and 326 more: https://git.openjdk.java.net/jdk/compare/16e83058cab4dd4d4a3fba812c8fe50e4286bd22...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 (@dfuch, @LanceAndersen, @AlanBateman, @mlchung) 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 Sep 24, 2021
@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 25, 2021

/integrate

@openjdk openjdk bot added the sponsor label Sep 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2021

@shiyuexw
Your change (at version 51e169d) is now ready to be sponsored by a Committer.

@huishi-hs
Copy link

@huishi-hs huishi-hs commented Sep 26, 2021

/sponsor

@shiyuexw shiyuexw closed this Sep 26, 2021
@shiyuexw shiyuexw reopened this Sep 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 26, 2021

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

  • 5ec1cdc: 8274321: Standardize values of @SInCE tags in javax.lang.model
  • 4838a2c: 8274143: Disable "invalid entry for security.provider.X" error message in log file when security.provider.X is empty
  • ab28db1: 8274312: ProblemList 2 serviceability/dcmd/gc tests with ZGC on macos-all
  • 8c122af: 8274314: Typo in WatchService#poll(long timeout, TimeUnit unit) javadoc
  • 9bc865d: 8273960: Redundant condition in Metadata.TypeComparator.compare
  • 5756385: 8274273: Update testing docs for MacOS with Non-US locale
  • 61ac53f: 8210927: JDB tests do not update source path after doing a redefine class
  • 341de49: 8273492: Move evacuation failure handling into G1YoungCollector
  • 13e9ea9: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125
  • 753b256: 8274296: Update or Problem List tests which may fail with uiScale=2 on macOS
  • ... and 331 more: https://git.openjdk.java.net/jdk/compare/16e83058cab4dd4d4a3fba812c8fe50e4286bd22...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 26, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Sep 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 26, 2021

@huishi-hs @shiyuexw Pushed as commit 7700b25.

💡 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
core-libs integrated
6 participants