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: Remove JarIndex support in URLClassPath #5383

Closed
wants to merge 2 commits into from

Conversation

shiyuexw
Copy link
Contributor

@shiyuexw shiyuexw commented Sep 7, 2021

There is a bug for URLClassPath.findResources with JarIndex.
With some discussions about the bug, the current priority is to remove the JAR index support in URLClassPath,
and don’t need to do anything to the jar tool in the short term, except just to move JarIndex to the jdk.jartool module.

The PR includes:

  1. remove the JarIndex support in URLClassPath
  2. move JarIndex into jdk.jartool module.

Progress

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

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8273401

Issue

  • JDK-8273401: Disable JarIndex Support In URLClassPath ⚠️ Title mismatch between PR and JBS. ⚠️ Issue is not open.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5383

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 7, 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 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 7, 2021

@shiyuexw The following labels will be automatically applied to this pull request:

  • compiler
  • core-libs
  • security

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

@openjdk openjdk bot added security core-libs compiler labels Sep 7, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 7, 2021

Webrevs

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 7, 2021

/label remove compiler

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 7, 2021

/csr

@openjdk openjdk bot removed the compiler label Sep 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 7, 2021

@AlanBateman
The compiler label was successfully removed.

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

@openjdk openjdk bot commented Sep 7, 2021

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@shiyuexw please create a CSR request for issue JDK-8273401. This pull request cannot be integrated until the CSR request is approved.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Thanks for taking this on. I've done a pass over the changes and it looks complete, just a few issues. Next step will be to create a CSR for this change.

/**
* The index file name.
*/
public static final String INDEX_NAME = "META-INF/INDEX.LIST";
Copy link
Contributor

@AlanBateman AlanBateman Sep 7, 2021

Choose a reason for hiding this comment

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

Adding this as a public field means it becomes part of the API, so it shouldn't be public here.

Copy link
Contributor

@LanceAndersen LanceAndersen Sep 7, 2021

Choose a reason for hiding this comment

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

Agree

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 8, 2021

Choose a reason for hiding this comment

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

remove public,but recover the same definition in JarIndex

@@ -145,7 +144,7 @@ public void beginEntry(JarEntry je, ManifestEntryVerifier mev)
}

if (uname.equals(JarFile.MANIFEST_NAME) ||
uname.equals(JarIndex.INDEX_NAME)) {
uname.equals(JarFile.INDEX_NAME)) {
Copy link
Contributor

@AlanBateman AlanBateman Sep 7, 2021

Choose a reason for hiding this comment

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

It would be useful if someone from security-libs could comment on this. The interaction between signed JAR and JAR index isn't very clear. The change you have is safe but it might be that we can drop the checking for INDEX.LIST here.

Copy link
Member

@seanjmullan seanjmullan Sep 7, 2021

Choose a reason for hiding this comment

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

I am thinking this line should not be removed for compatibility with existing JARs that have indexes.

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 8, 2021

Choose a reason for hiding this comment

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

still keep the code

@@ -39,7 +39,7 @@
import java.util.jar.JarEntry;
import java.util.jar.JarOutputStream;
// implementation specific API
import jdk.internal.util.jar.JarIndex;
import sun.tools.jar.JarIndex;

public class JarIndexMergeTest {
Copy link
Contributor

@AlanBateman AlanBateman Sep 7, 2021

Choose a reason for hiding this comment

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

Is this the one remaining test in sun/misc/JarIndex? I think it can be moved to test/jdk/java/util/jar. I think we should change the package name in the summary comment too.

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 8, 2021

Choose a reason for hiding this comment

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

This is the only test case, and I move it to test/jdk/java/util/jar.
The summary comment is also changed.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

I think this looks good overall.

Once you do your next set up updates, I will run it through mach5 for you

/**
* The index file name.
*/
public static final String INDEX_NAME = "META-INF/INDEX.LIST";
Copy link
Contributor

@LanceAndersen LanceAndersen Sep 7, 2021

Choose a reason for hiding this comment

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

Agree

@@ -25,7 +25,7 @@
* @test
Copy link
Contributor

@LanceAndersen LanceAndersen Sep 7, 2021

Choose a reason for hiding this comment

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

Probably should update the copyright year to include 2021

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 8, 2021

Choose a reason for hiding this comment

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

The copyright year includes 2021.


import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
import static java.util.jar.JarFile.MANIFEST_NAME;
import static java.util.stream.Collectors.joining;
import static jdk.internal.util.jar.JarIndex.INDEX_NAME;
import static java.util.jar.JarFile.INDEX_NAME;
Copy link
Contributor

@LanceAndersen LanceAndersen Sep 7, 2021

Choose a reason for hiding this comment

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

Maybe move this above the java.util.streamCollectors.joining import

Also, the copyright should probably be updated to 2021

Copy link
Contributor Author

@shiyuexw shiyuexw Sep 8, 2021

Choose a reason for hiding this comment

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

update to 2021

Include:
1. remove public for INDEX_NAME in JarFile
2. recover the definition for INDEX_NAME in JarIndex
3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar
@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 8, 2021

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Sep 8, 2021

Hi all,

Just wondering why we remove JarIndex other than fixing it? Is there any strong motive that drives us to do this?

Judging from our internal performance testing and user feedback, JarIndex can significantly reduce the time for class/resource lookup. Although JarIndex is an ancient technology, it is still helpful for many modern scenarios such as serverless applications.

Thanks.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 8, 2021

Create CSR: https://bugs.openjdk.java.net/browse/JDK-8273473

Thanks. I've updated the CSR to make it clearer what this change is about. There is still some TDB for the JAR file spec.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 14, 2021

@shiyuexw I discussed this issue with other maintainers in this area. 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. I've updated the CSR to reflect this proposal. Are you okay to continue with this new proposal? It would mean that we wouldn't remove the tests but instead change them to run with the system property.

@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 15, 2021

@shiyuexw I discussed this issue with other maintainers in this area. 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. I've updated the CSR to reflect this proposal. Are you okay to continue with this new proposal? It would mean that we wouldn't remove the tests but instead change them to run with the system property.

Yes, I will take care of it. Need I create a new PR?

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 15, 2021

Yes, I will take care of it. Need I create a new PR?

Up to you, it's okay to continue with this one if you want, we would just need to change the description here and in JBS.

@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 15, 2021

Yes, I will take care of it. Need I create a new PR?

Up to you, it's okay to continue with this one if you want, we would just need to change the description here and in JBS.

I created a new PR #5524 to disable JarIndex and update the description in JBS.

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

@openjdk openjdk bot commented Sep 26, 2021

@shiyuexw 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 removeindex
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict label Sep 26, 2021
@shiyuexw
Copy link
Contributor Author

@shiyuexw shiyuexw commented Sep 27, 2021

Firstly, disable JarIndex support in URLClassPath. So close the PR.

@shiyuexw shiyuexw closed this Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs merge-conflict rfr security
5 participants