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

JDK-8281001 Class::forName(String) defaults to system class loader if the caller is null #8711

Closed
wants to merge 9 commits into from

Conversation

tprinzing
Copy link
Contributor

@tprinzing tprinzing commented May 14, 2022

The Class::forName behavior change to match JNI FindClass is a compatible change and seems pretty attractive as it would be expected that Class::forName would give the same behavior as FindClass which uses the system classloader. The test for 8281006 was enhanced to test for this change. Merged master to pick up fixes to unrelated test failures to reduce noise.


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-8281001: Class::forName(String) defaults to system class loader if the caller is null
  • JDK-8286927: Class::forName(String) defaults to system class loader if the caller is null (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8711

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 14, 2022

👋 Welcome back tprinzing! 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 May 14, 2022
@openjdk
Copy link

openjdk bot commented May 14, 2022

@tprinzing 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 May 14, 2022
@mlbridge
Copy link

mlbridge bot commented May 14, 2022

Webrevs

@jddarcy
Copy link
Member

jddarcy commented May 14, 2022

/csr needed

@jddarcy
Copy link
Member

jddarcy commented May 14, 2022

@tprinzing , please file a CSR for this discuss so the behavioral changes can be reviewed.

@openjdk openjdk bot added the csr label May 14, 2022
@openjdk
Copy link

openjdk bot commented May 14, 2022

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

Copy link
Member

@mlchung mlchung left a comment

Class::forName(String) javadoc should specify which class loader to use when invoked with null caller similar to the other APIs you fixed for null callers.

I think this new test case does not belong to NullCallerGetResource.java which is a test for Module::getResource. It's better to be placed under the test/jdk/java/lang/Class/forName directory that makes it clear it's a test for Class::forName.

Alternatively, we could move all the tests for null caller under a new clearly-named directory, maybe test/jdk/jdk/nullCaller. This may allow to do some refactoring and remove the duplicated code in these test cases. What do you think?

@@ -25,6 +25,7 @@
/**
* @test
* @bug 8281006
* @bug 8281001
Copy link
Member

@mlchung mlchung May 16, 2022

Choose a reason for hiding this comment

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

Suggested change
* @bug 8281001
* @bug 8281006 8281001

@bug can have one or more bug IDs

return forName0(className, true, ClassLoader.getClassLoader(caller), caller);
ClassLoader loader = (caller == null) ?
ClassLoader.getSystemClassLoader() :
ClassLoader.getClassLoader(caller);
Copy link
Member

@mlchung mlchung May 16, 2022

Choose a reason for hiding this comment

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

Formatting nit:

   ClassLoader loader = (caller == null) ? ClassLoader.getSystemClassLoader()
                                         : ClassLoader.getClassLoader(caller);

@iklam
Copy link
Member

iklam commented May 16, 2022

Please rename the title of the issue to reflect what is being proposed.

@tprinzing
Copy link
Contributor Author

tprinzing commented May 17, 2022

Class::forName(String) javadoc should specify which class loader to use when invoked with null caller similar to the other APIs you fixed for null callers.

I think this new test case does not belong to NullCallerGetResource.java which is a test for Module::getResource. It's better to be placed under the test/jdk/java/lang/Class/forName directory that makes it clear it's a test for Class::forName.

Alternatively, we could move all the tests for null caller under a new clearly-named directory, maybe test/jdk/jdk/nullCaller. This may allow to do some refactoring and remove the duplicated code in these test cases. What do you think?

I like the idea of moving all the null caller tests to a clearly named directory to avoid duplication.

@mlchung
Copy link
Member

mlchung commented May 17, 2022

I like the idea of moving all the null caller tests to a clearly named directory to avoid duplication.

+1. You can do this refactoring in a separate JBS issue and then update this PR when the tests are moved.

@@ -357,6 +357,12 @@ static String typeVarBounds(TypeVariable<?> typeVar) {
* A call to {@code forName("X")} causes the class named
* {@code X} to be initialized.
*
* <p>
* This method is caller sensitive. In cases where this method is called
Copy link
Member

@mlchung mlchung May 25, 2022

Choose a reason for hiding this comment

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

You can drop "This method is caller sensitive." sentence for consistency with other caller-sensitive methods that do not state that explicitly. The javadoc already specifies that it uses the class loader of the current class.

@openjdk openjdk bot removed the csr label May 26, 2022
tprinzing added 2 commits Jun 2, 2022
the c++ std library as part of the build.  Changed the test to
use iostream instead of printf.  Enabled the test for Class::forName
which is now located in test/jdk/jni/nullCaller (as part of the
merge of JDK-8287171).
@tprinzing tprinzing changed the title JDK-8281001 Examine the behavior of Class::forName if the caller is null JDK-8281001 Class::forName(String) defaults to system class loader if the caller is null Jun 3, 2022
@@ -24,7 +24,7 @@

/**
* @test
* @bug 8280902 8281000 8281003 8281006
* @bug 8280902 8281000 8281001 8281003 8281006
Copy link
Member

@mlchung mlchung Jun 8, 2022

Choose a reason for hiding this comment

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

nit: append the bug rather than keeping the list in an increasing order.

mlchung
mlchung approved these changes Jun 8, 2022
Copy link
Member

@mlchung mlchung left a comment

Looks good. Thanks for refactoring the tests, making the addition of a new test case much cleaner.

@openjdk
Copy link

openjdk bot commented Jun 8, 2022

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

8281001: Class::forName(String) defaults to system class loader if the caller is null

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

  • c68419f: 8286990: Add compiler name to warning messages in Compiler Directive
  • 6fb84e2: 8287541: Files.writeString fails to throw IOException for charset "windows-1252"
  • a9b9831: 8286663: Resolve IDE warnings in WTrayIconPeer and SystemTray
  • b021d37: 8283383: [macos] a11y : Screen magnifier shows extra characters (0) at the end JButton accessibility name
  • 78d3712: 8287432: C2: assert(tn->in(0) != __null) failed: must have live top node
  • f7791ad: 8287895: Some langtools tests fail on msys2
  • 5ad6286: 8287970: riscv: jdk/incubator/vector/*VectorTests failing
  • a9d46f3: 8287894: Use fixed timestamp as an alternative of DATE macro in jdk.jdi to make Windows build reproducible
  • 6e3e470: 8285965: TestScenarios.java does not check for "" correctly
  • d959c22: 8288000: compiler/loopopts/TestOverUnrolling2.java fails with release VMs
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/8e0783917975075aae5d586f0076d5093afb0b62...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 (@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 Jun 8, 2022
@tprinzing
Copy link
Contributor Author

tprinzing commented Jun 8, 2022

/integrate

@openjdk openjdk bot added the sponsor label Jun 8, 2022
@openjdk
Copy link

openjdk bot commented Jun 8, 2022

@tprinzing
Your change (at version 95d7044) is now ready to be sponsored by a Committer.

mlchung
mlchung approved these changes Jun 8, 2022
@mlchung
Copy link
Member

mlchung commented Jun 8, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 8, 2022

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

  • c68419f: 8286990: Add compiler name to warning messages in Compiler Directive
  • 6fb84e2: 8287541: Files.writeString fails to throw IOException for charset "windows-1252"
  • a9b9831: 8286663: Resolve IDE warnings in WTrayIconPeer and SystemTray
  • b021d37: 8283383: [macos] a11y : Screen magnifier shows extra characters (0) at the end JButton accessibility name
  • 78d3712: 8287432: C2: assert(tn->in(0) != __null) failed: must have live top node
  • f7791ad: 8287895: Some langtools tests fail on msys2
  • 5ad6286: 8287970: riscv: jdk/incubator/vector/*VectorTests failing
  • a9d46f3: 8287894: Use fixed timestamp as an alternative of DATE macro in jdk.jdi to make Windows build reproducible
  • 6e3e470: 8285965: TestScenarios.java does not check for "" correctly
  • d959c22: 8288000: compiler/loopopts/TestOverUnrolling2.java fails with release VMs
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/8e0783917975075aae5d586f0076d5093afb0b62...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jun 8, 2022
@openjdk openjdk bot closed this Jun 8, 2022
@openjdk openjdk bot removed ready rfr sponsor labels Jun 8, 2022
@openjdk
Copy link

openjdk bot commented Jun 8, 2022

@mlchung @tprinzing Pushed as commit b92ce26.

💡 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
4 participants