Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v7]
On Wed, 16 Feb 2022 21:39:20 GMT, Tim Prinzing wrote: >> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is >> null > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge master > - fix test breakage from rename > - reformat test summary > - reformat test summary > - improve test name and remove experimental test code > - fix copyright date > - More changes from feedback. > >The javadoc is improved to reflect InvalidCallerException is thrown with >a caller that can't be assigned to a ClassLoader as well as a null >caller frame. Added a test IsParallelCapableBadCaller that uses >reflection hackery to create a case where called with an invalid caller >on the call stack. > - Changes from feedback. > >- Copyright dates fixed >- IllegalCallerException thrown for no caller frame, and associated >javadoc changes >- test changed to look for IllegalCallerException thrown. > - Merge branch 'master' into JDK-8281000 > - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Looks fine. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v7]
On Wed, 16 Feb 2022 21:39:20 GMT, Tim Prinzing wrote: >> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is >> null > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge master > - fix test breakage from rename > - reformat test summary > - reformat test summary > - improve test name and remove experimental test code > - fix copyright date > - More changes from feedback. > >The javadoc is improved to reflect InvalidCallerException is thrown with >a caller that can't be assigned to a ClassLoader as well as a null >caller frame. Added a test IsParallelCapableBadCaller that uses >reflection hackery to create a case where called with an invalid caller >on the call stack. > - Changes from feedback. > >- Copyright dates fixed >- IllegalCallerException thrown for no caller frame, and associated >javadoc changes >- test changed to look for IllegalCallerException thrown. > - Merge branch 'master' into JDK-8281000 > - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v7]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge master - fix test breakage from rename - reformat test summary - reformat test summary - improve test name and remove experimental test code - fix copyright date - More changes from feedback. The javadoc is improved to reflect InvalidCallerException is thrown with a caller that can't be assigned to a ClassLoader as well as a null caller frame. Added a test IsParallelCapableBadCaller that uses reflection hackery to create a case where called with an invalid caller on the call stack. - Changes from feedback. - Copyright dates fixed - IllegalCallerException thrown for no caller frame, and associated javadoc changes - test changed to look for IllegalCallerException thrown. - Merge branch 'master' into JDK-8281000 - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null - Changes: https://git.openjdk.java.net/jdk/pull/7448/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=06 Stats: 216 lines in 5 files changed: 216 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v6]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: fix test breakage from rename - Changes: - all: https://git.openjdk.java.net/jdk/pull/7448/files - new: https://git.openjdk.java.net/jdk/pull/7448/files/a449acdc..977162db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v5]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request incrementally with three additional commits since the last revision: - reformat test summary - reformat test summary - improve test name and remove experimental test code - Changes: - all: https://git.openjdk.java.net/jdk/pull/7448/files - new: https://git.openjdk.java.net/jdk/pull/7448/files/533a0660..a449acdc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=03-04 Stats: 129 lines in 3 files changed: 58 ins; 70 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v4]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: fix copyright date - Changes: - all: https://git.openjdk.java.net/jdk/pull/7448/files - new: https://git.openjdk.java.net/jdk/pull/7448/files/d67bbb16..533a0660 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v3]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: More changes from feedback. The javadoc is improved to reflect InvalidCallerException is thrown with a caller that can't be assigned to a ClassLoader as well as a null caller frame. Added a test IsParallelCapableBadCaller that uses reflection hackery to create a case where called with an invalid caller on the call stack. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7448/files - new: https://git.openjdk.java.net/jdk/pull/7448/files/fa66af15..d67bbb16 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=01-02 Stats: 85 lines in 3 files changed: 74 ins; 5 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v2]
On Tue, 15 Feb 2022 23:05:30 GMT, Mandy Chung wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Changes from feedback.
>>
>> - Copyright dates fixed
>> - IllegalCallerException thrown for no caller frame, and associated
>> javadoc changes
>> - test changed to look for IllegalCallerException thrown.
>
> src/java.base/share/classes/java/lang/ClassLoader.java line 1626:
>
>> 1624: protected static boolean registerAsParallelCapable() {
>> 1625: final Class caller = Reflection.getCallerClass();
>> 1626: if (caller == null) {
>
> Suggestion:
>
> if (caller == null || !ClassLoader.class.isAssignableFrom(caller)) {
> throw new IllegalCallerException(caller + " not a subclass of
> ClassLoader");
> }
>
>
> What we suggested is to throw IllegalCallerException if the caller is not a
> class loader and that will include null caller case.
This also needs a test case for non-class loader caller.
I looked at the existing tests testing `registerAsParallelCapable` but I can't
find a good one to add the new test case. The closest one could be
`test/jdk/java/lang/ClassLoader/IsParallelCapable.java`. I'm okay to include
a new test case in `IsParallelCapable.java` to verify `IllegalCallerException`
if called by a non-class loader.
-
PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v2]
On Tue, 15 Feb 2022 22:17:53 GMT, Tim Prinzing wrote:
>> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is
>> null
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changes from feedback.
>
> - Copyright dates fixed
> - IllegalCallerException thrown for no caller frame, and associated
> javadoc changes
> - test changed to look for IllegalCallerException thrown.
src/java.base/share/classes/java/lang/ClassLoader.java line 1612:
> 1610: * In cases where {@code ClassLoader.registerAsParallelCapable} is
> called from a context where
> 1611: * there is no caller frame on the stack (e.g. when called directly
> 1612: * from a JNI attached thread), {@code IllegalCallerException} is
> thrown.
Suggestion:
* In cases where this method is called from a context where the caller is
not a subclass
* {@code ClassLoader} or there is no caller frame on the stack (e.g. when
called directly
* from a JNI attached thread), {@code IllegalCallerException} is thrown.
Should mention the non-class loader caller as well.
src/java.base/share/classes/java/lang/ClassLoader.java line 1617:
> 1615: * @return {@code true} if the caller is successfully registered as
> 1616: * parallel capable and {@code false} if otherwise.
> 1617: * @throws IllegalCallerException if there is no caller frame on
> the stack.
Suggestion:
* @throws IllegalCallerException if the caller is not a subclass of {@code
ClassLoader}
src/java.base/share/classes/java/lang/ClassLoader.java line 1626:
> 1624: protected static boolean registerAsParallelCapable() {
> 1625: final Class caller = Reflection.getCallerClass();
> 1626: if (caller == null) {
Suggestion:
if (caller == null || !ClassLoader.class.isAssignableFrom(caller)) {
throw new IllegalCallerException(caller + " not a subclass of
ClassLoader");
}
What we suggested is to throw IllegalCallerException if the caller is not a
class loader and that will include null caller case.
test/jdk/java/lang/ClassLoader/exeNullCallerClassLoaderTest/NullCallerClassLoaderTest.java
line 30:
> 28: * @summary Test uses custom launcher that starts VM using JNI that
> verifies
> 29: * ClassLoader.registerAsParallelCapable with null caller class
> does not throw a NullPointerException,
> 30: * and instead returns false.
`@summary` needs update to reflect the new change.
-
PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v2]
> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Changes from feedback. - Copyright dates fixed - IllegalCallerException thrown for no caller frame, and associated javadoc changes - test changed to look for IllegalCallerException thrown. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7448/files - new: https://git.openjdk.java.net/jdk/pull/7448/files/c6d37069..fa66af15 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=00-01 Stats: 33 lines in 3 files changed: 22 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Build changes LGTM. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 23:25:44 GMT, Brent Christian wrote: > Having a second thought, since this API expects to be called by a class > loader, I think throwing `IllegalCallerException` to indicate this method is > called by an illegal caller. This will need a CSR due to the spec change. I think this would work for both the "no caller" case and also the case where there is reflection hackery calling this method from somewhere other than a ClassLoader. So it would be a small change in behavior from CCE to ICE. - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Yes, I like the IllegalCallerException. - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 22:37:57 GMT, Mandy Chung wrote: > Thanks for taking on these null caller issue. > > To give more context to this issue, the spec of > `ClassLoader::isRegisteredAsParallelCapable` returns true if this class > loader is registered as parallel capable, otherwise false. The current spec > does not specify what if the caller class is not a class loader. The current > implementation throws NPE if caller is null. I initially proposed to return > false for simplicity. However, if the caller is not a subclass of > `ClassLoader`, the current implementation throws `ClassCastException`. Both > cases are invalid caller and they should be handled in the same way, either > return false or throw an exception. > > Having a second thought, since this API expects to be called by a class > loader, I think throwing `IllegalCallerException` to indicate this method is > called by an illegal caller. This will need a CSR due to the spec change. > > What do you think? Throwing IllegalCallerException sounds good to me. As a static method, from a language standpoint, the call could appear almost anywhere. But its intended usage is pretty narrow. I think it's worth notifying the developer of a pretty obvious error. Is this method mainly meant to be called from a classloader's static initializer? That might be worth mentioning in the JavaDoc (if we're doing a CSR anyway...). - PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Build change looks good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7448
Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing wrote: > JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is > null Thanks for taking on these null caller issue. To give more context to this issue, the spec of `ClassLoader::isRegisteredAsParallelCapable` returns true if this class loader is registered as parallel capable, otherwise false. The current spec does not specify what if the caller class is not a class loader. The current implementation throws NPE if caller is null. I initially proposed to return false for simplicity. However, if the caller is not a subclass of `ClassLoader`, the current implementation throws `ClassCastException`. Both cases are invalid caller and they should be handled in the same way, either return false or throw an exception. Having a second thought, since this API expects to be called by a class loader, I think throwing `IllegalCallerException` to indicate this method is called by an illegal caller.This will need a CSR due to the spec change. What do you think? - PR: https://git.openjdk.java.net/jdk/pull/7448
RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null
JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null - Commit messages: - Merge branch 'master' into JDK-8281000 - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null Changes: https://git.openjdk.java.net/jdk/pull/7448/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7448&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281000 Stats: 140 lines in 4 files changed: 139 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7448.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7448/head:pull/7448 PR: https://git.openjdk.java.net/jdk/pull/7448
