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

8263632: Improve exception handling of APIs in classLoader.cpp #3203

Conversation

@calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Mar 25, 2021

Please review this change which includes:

  • adding ClassLoader::create_class_path_entry_or_fail() and ClassLoader::create_class_path_entry_or_fail() functions for better readability. They will call the existing ClassLoader::create_class_path_entry().
  • replacing the TRAPS parameter with Thread* current for the functions which never throw exception.

Testing: tiers 1,2 (passed); tiers 3,4 (in progress).


Progress

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

Issue

  • JDK-8263632: Improve exception handling of APIs in classLoader.cpp

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/3203
$ git pull https://git.openjdk.java.net/jdk pull/3203/head

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 25, 2021

/label add hotspot-runtime

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 25, 2021

👋 Welcome back ccheung! 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.

@calvinccheung calvinccheung marked this pull request as ready for review Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the rfr label Mar 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 25, 2021

@@ -551,7 +551,7 @@ void ClassLoader::setup_module_search_path(const char* path, TRAPS) {
}
// File or directory found
ClassPathEntry* new_entry = NULL;
new_entry = create_class_path_entry(path, &st, true /* throw_exception */,
new_entry = create_class_path_entry_or_fail(path, &st,

This comment has been minimized.

@iklam

iklam Mar 25, 2021
Member

Ideally, there's should be no need to check for new_entry == NULL when calling calling create_class_path_entry_or_fail(). This will make the code easier to read.

But currently the create_class_path_entry_or_fail() is still "tri-state":

  • success: non-null
  • throw vmSymbols::java_io_IOException()
  • return NULL

The third case is this:

        // Don't complain about bad jar files added via -Xbootclasspath/a:.
        if (throw_exception && is_init_completed()) {
          THROW_MSG_(vmSymbols::java_lang_ClassNotFoundException(), msg, NULL);
        } else {
          return NULL; <<<<< here
        }

I am wondering if we can get rid of this logic. That way, we can add asserts like:

ClassPathEntry* ClassLoader::create_class_path_entry_or_fail(...) {
  ClassPathEntry* entry = create_class_path_entry(, THREAD);
  assert(entry != NULL || HAS_PENDING_EXCEPTION, "must throw or return valid entry");
  return entry;
}

ClassPathEntry* ClassLoader::create_class_path_entry_or_null(...) {
  ClassPathEntry* entry = create_class_path_entry(, current);
  assert(!HAS_PENDING_EXCEPTION, "must not throw");
  return entry;
}

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

You can't use HAS_PENDING_EXCEPTION in create_class_path_entry_or_null without reintroducing THREAD.

This comment has been minimized.

@calvinccheung

calvinccheung Mar 26, 2021
Author Member

After offline discussion with Ioi, we've decided to get rid of the throw_exception in create_class_path_entry(). This makes the code cleaner. We could also drop the TRAPS parameter of a few other related functions.

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 29, 2021
Member

It is not at all clear to me how you now deal with the error cases without exceptions. How is it detected that ClassLoader::setup_module_search_path failed for example?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

LGTM!

Thanks,
David

@openjdk
Copy link

@openjdk openjdk bot commented Mar 26, 2021

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

8263632: Improve exception handling of APIs in classLoader.cpp

Reviewed-by: iklam, dholmes, coleenp

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

  • 7284f01: 8262110: DST starts from incorrect time in 2038
  • 3a28dc8: 8264178: Unused method Threads::nmethods_do
  • 33c94ff: 8263376: CTW (Shenandoah): assert(mems <= 1) failed: No node right after call if multiple mem projections
  • 4e74de4: 8264111: (fs) Leaking NativeBuffers in case of errors during UnixUserDefinedFileAttributeView.read/write
  • 57115fa: 8189198: Add "forRemoval = true" to Applet API deprecations
  • b8122d6: 8264220: jdk/javadoc/doclet/testRelatedPackages/TestRelatedPackages.java fails to compile
  • 507b690: 8264126: Remove TRAPS/THREAD parameter for class loading functions
  • f3eed05: 8263928: Add JAWT test files for mac
  • 4fbb7c2: 8263472: Specification of JComponent::updateUI should document that the default implementation does nothing
  • e47dfb8: 8264062: Use the blessed modifier order in jdk.jfr
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/37f494cafad95a36ec368b48bbebf5bc54c8cbe8...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 26, 2021
@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 26, 2021

LGTM!

Thanks,
David

Thanks David.

Copy link

@coleenp coleenp left a comment

Looks good.

}

#if INCLUDE_CDS
void ClassLoader::setup_app_search_path(const char *class_path, TRAPS) {
void ClassLoader::setup_app_search_path(Thread* current, const char *class_path) {
Arguments::assert_is_dumping_archive();

ResourceMark rm;

This comment has been minimized.

@coleenp

coleenp Mar 26, 2021

You can add 'current' as a parameter to ResourceMark since you have it.

This comment has been minimized.

@calvinccheung

calvinccheung Mar 26, 2021
Author Member

Thanks Coleen.
I've fixed it.

@iklam
iklam approved these changes Mar 26, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. Just one small nit.

}

add_to_module_path_entries(path, new_entry);
return;

This comment has been minimized.

@iklam

iklam Mar 26, 2021
Member

nit: return is no longer needed.

This comment has been minimized.

@calvinccheung

calvinccheung Mar 26, 2021
Author Member

Thanks Ioi.
I've fixed it.

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 26, 2021

/integrate

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

@openjdk openjdk bot commented Mar 26, 2021

@calvinccheung Since your change was applied there have been 38 commits pushed to the master branch:

  • 59ed1fa: 8264087: Use the blessed modifier order in jdk.jconsole
  • 054e0a4: 8264017: Correctly report inlined frame in JFR sampling
  • d6bb153: 8264240: [macos_aarch64] enable appcds support after JDK-8263002
  • 7284f01: 8262110: DST starts from incorrect time in 2038
  • 3a28dc8: 8264178: Unused method Threads::nmethods_do
  • 33c94ff: 8263376: CTW (Shenandoah): assert(mems <= 1) failed: No node right after call if multiple mem projections
  • 4e74de4: 8264111: (fs) Leaking NativeBuffers in case of errors during UnixUserDefinedFileAttributeView.read/write
  • 57115fa: 8189198: Add "forRemoval = true" to Applet API deprecations
  • b8122d6: 8264220: jdk/javadoc/doclet/testRelatedPackages/TestRelatedPackages.java fails to compile
  • 507b690: 8264126: Remove TRAPS/THREAD parameter for class loading functions
  • ... and 28 more: https://git.openjdk.java.net/jdk/compare/37f494cafad95a36ec368b48bbebf5bc54c8cbe8...master

Your commit was automatically rebased without conflicts.

Pushed as commit c9d2d02.

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

@calvinccheung calvinccheung deleted the calvinccheung:8263632-exp-handling-classLoader branch Mar 26, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Mailing list message from Ioi Lam on hotspot-runtime-dev:

On 3/28/21 9:40 PM, David Holmes wrote:

On Fri, 26 Mar 2021 16:12:54 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

You can't use HAS_PENDING_EXCEPTION in create_class_path_entry_or_null without reintroducing THREAD.
After offline discussion with Ioi, we've decided to get rid of the `throw_exception` in `create_class_path_entry()`. This makes the code cleaner. We could also drop the `TRAPS` parameter of a few other related functions.
It is not at all clear to me how you now deal with the error cases without exceptions. How is it detected that ClassLoader::setup_module_search_path failed for example?

During VM bootstrap, classLoader.cpp tries to open files on related to
-Xbootclasspath/a: and --patch-module.

For --patch-module handling, throw_exception=false was specified.

For bootclasspath handling, throw_exception=true was specified before
JDK-8263632:

https://github.com/openjdk/jdk/blame/59ed1fa28c80395ddc5e8d8611e2225349aaff1a/src/hotspot/share/classfile/classLoader.cpp#L672

?? ClassLoader::initialize
?? -> ClassLoader::setup_bootstrap_search_path
?? -> ClassLoader::setup_bootstrap_search_path_impl
?? -> update_class_path_entry_list
?? -> create_class_path_entry(path, &st, /*throw_exception=*/true, ...)

But this will not throw any exception for invalid JAR files:

?? [1] the file doesn't exist.
?? $ rm -f foo.jar
?? $ java -Xbootclasspath/a:foo.jar -version
?? openjdk version "15" 2020-09-15
?? OpenJDK Runtime Environment (build 15+36-1562)
?? OpenJDK 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)

?? [2] the file exists but is not a ZIP file
?? $ touch foo.jar
?? $ java -Xbootclasspath/a:foo.jar -version
?? openjdk version "15" 2020-09-15
?? OpenJDK Runtime Environment (build 15+36-1562)
?? OpenJDK 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)

The behavior of [2] is due to this: is_init_completed() is false during
VM bootstrap.

?? // Don't complain about bad jar files added via -Xbootclasspath/a:.
?? if (throw_exception && is_init_completed()) {
???? THROW_MSG_(vmSymbols::java_lang_ClassNotFoundException(), msg, NULL);
?? } else {
???? return NULL;
?? }

create_class_path_entry() is called when is_init_completed() is true
only during CDS dumping code. We decided that we don't need the
exception in this case and will just ignore any non-existing or invalid
ZIP files. Getting rid of the throw_exception parameter makes things a
lot simpler.

Thanks
- Ioi

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 30, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 30/03/2021 9:01 am, Ioi Lam wrote:

On 3/28/21 9:40 PM, David Holmes wrote:

On Fri, 26 Mar 2021 16:12:54 GMT, Calvin Cheung <ccheung at openjdk.org>
wrote:

You can't use HAS_PENDING_EXCEPTION in
create_class_path_entry_or_null without reintroducing THREAD.
After offline discussion with Ioi, we've decided to get rid of the
`throw_exception` in `create_class_path_entry()`. This makes the code
cleaner. We could also drop the `TRAPS` parameter of a few other
related functions.
It is not at all clear to me how you now deal with the error cases
without exceptions. How is it detected that
ClassLoader::setup_module_search_path failed for example?

During VM bootstrap, classLoader.cpp tries to open files on related to
-Xbootclasspath/a: and --patch-module.

For --patch-module handling, throw_exception=false was specified.

For bootclasspath handling, throw_exception=true was specified before
JDK-8263632:

https://github.com/openjdk/jdk/blame/59ed1fa28c80395ddc5e8d8611e2225349aaff1a/src/hotspot/share/classfile/classLoader.cpp#L672

?? ClassLoader::initialize
?? -> ClassLoader::setup_bootstrap_search_path
?? -> ClassLoader::setup_bootstrap_search_path_impl
?? -> update_class_path_entry_list
?? -> create_class_path_entry(path, &st, /*throw_exception=*/true, ...)

But this will not throw any exception for invalid JAR files:

?? [1] the file doesn't exist.
?? $ rm -f foo.jar
?? $ java -Xbootclasspath/a:foo.jar -version
?? openjdk version "15" 2020-09-15
?? OpenJDK Runtime Environment (build 15+36-1562)
?? OpenJDK 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)

?? [2] the file exists but is not a ZIP file
?? $ touch foo.jar
?? $ java -Xbootclasspath/a:foo.jar -version
?? openjdk version "15" 2020-09-15
?? OpenJDK Runtime Environment (build 15+36-1562)
?? OpenJDK 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)

The behavior of [2] is due to this: is_init_completed() is false during
VM bootstrap.

?? // Don't complain about bad jar files added via -Xbootclasspath/a:.
?? if (throw_exception && is_init_completed()) {
???? THROW_MSG_(vmSymbols::java_lang_ClassNotFoundException(), msg, NULL);
?? } else {
???? return NULL;
?? }

create_class_path_entry() is called when is_init_completed() is true
only during CDS dumping code. We decided that we don't need the
exception in this case and will just ignore any non-existing or invalid
ZIP files. Getting rid of the throw_exception parameter makes things a
lot simpler.

Thanks for the explanations Ioi!

David
-----

Thanks
- Ioi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants