RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error
Hi, Please review the following change: Bug: https://bugs.openjdk.java.net/browse/JDK-8187222 Webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.00/ The method description of ClassLoader.getSystemClassLoader() states (in regards to setting a custom system classloader): "If circular initialization of the system class loader is detected then an unspecified error or exception is thrown." This ambiguity in the method description can and should be removed. The method also has a @throws tag: * @throws IllegalStateException * If invoked recursively during the construction of the class * loader specified by the "{@code java.system.class.loader}" * property. JDK 8 threw an IllegalStateException. This changed to an InternalError in 9b111[1]. Throwing an IllegalStateException conforms to the @throws tag, and returns to the previous behavior of JDK 8 and (earlier on in) 9. Also, as Alan points out in the bug, an InternalError looks like a bug in the JDK itself, whereas IllegalStateException better reflects that the issue likely lies in the custom classloader. Automated build & test job passes cleanly. Thanks, -Brent 1. https://bugs.openjdk.java.net/browse/JDK-8142968
On 11/30/17 1:35 PM, Brent Christian wrote:
Hi,
Please review the following change:
Bug: https://bugs.openjdk.java.net/browse/JDK-8187222 Webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.00/
Thanks for fixing it. This is indeed a bug that should throw ISE when ClassLoader.getSystemClassLoader is called during the initialization of the system class loader, as the spec states. line 1921: I suggest to revise the message to make it clearer: "getSystemClassLoader cannot be called during the system class loader instantiation" In ClassLoader::initSystemClassLoader (line 1971), when the exception is thrown via Constructor::newInstance, it would be good to the cause of an InvocationTargetException like: + Throwable t = e; + if (e instanceof InvocationTargetException) { + t = e.getCause(); + } + throw new Error(t.getMessage(), t); InitSystemLoaderTest.java verifies a working custom system class loader. This recursive custom system loader test fits better as a new test rather a test case in InitSystemLoaderTest.java. I suggest to rename ThrowingCustomLoader as RecursiveSystemLoader and make it as jtreg test with a static void main (maybe it should validate what getSystemClassLoader returns). It can be made simpler e.g. line 30-31, 45-47 and 54 are not needed. Mandy
On 30/11/2017 22:57, mandy chung wrote:
:
This is indeed a bug that should throw ISE when ClassLoader.getSystemClassLoader is called during the initialization of the system class loader, as the spec states.
line 1921: I suggest to revise the message to make it clearer: "getSystemClassLoader cannot be called during the system class loader instantiation"
In ClassLoader::initSystemClassLoader (line 1971), when the exception is thrown via Constructor::newInstance, it would be good to the cause of an InvocationTargetException like:
+ Throwable t = e; + if (e instanceof InvocationTargetException) { + t = e.getCause(); + } + throw new Error(t.getMessage(), t); Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
-Alan
On 12/1/17 2:32 AM, Alan Bateman wrote:
On 30/11/2017 22:57, mandy chung wrote:
:
This is indeed a bug that should throw ISE when ClassLoader.getSystemClassLoader is called during the initialization of the system class loader, as the spec states.
line 1921: I suggest to revise the message to make it clearer: "getSystemClassLoader cannot be called during the system class loader instantiation"
In ClassLoader::initSystemClassLoader (line 1971), when the exception is thrown via Constructor::newInstance, it would be good to the cause of an InvocationTargetException like:
+ Throwable t = e; + if (e instanceof InvocationTargetException) { + t = e.getCause(); + } + throw new Error(t.getMessage(), t); Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
Yes that would be better. Mandy
On 12/1/17 8:33 AM, mandy chung wrote:
Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
Yes that would be better.
So would I do this for RuntimeException and Error, and then package checked types of causes in an Error, as Mandy suggested? (We don't want to throw a checked exception from initSystemClassLoader(), I don't think.) Thanks, -Brent
On 12/1/17 11:40 AM, Brent Christian wrote:
On 12/1/17 8:33 AM, mandy chung wrote:
Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
Yes that would be better.
So would I do this for RuntimeException and Error, and then package checked types of causes in an Error, as Mandy suggested? (We don't want to throw a checked exception from initSystemClassLoader(), I don't think.)
Anyway, this is what that looks like (I did RuntimeException, but didn't bother with Error). I also made the test changes that Mandy suggested. http://cr.openjdk.java.net/~bchristi/8187222/webrev.01/index.html Thanks, -Brent
On Dec 1, 2017, at 4:46 PM, Brent Christian <brent.christian@oracle.com> wrote:
On 12/1/17 11:40 AM, Brent Christian wrote: On 12/1/17 8:33 AM, mandy chung wrote:
Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
Yes that would be better. So would I do this for RuntimeException and Error, and then package checked types of causes in an Error, as Mandy suggested? (We don't want to throw a checked exception from initSystemClassLoader(), I don't think.)
Anyway, this is what that looks like (I did RuntimeException, but didn't bother with Error). I also made the test changes that Mandy suggested.
http://cr.openjdk.java.net/~bchristi/8187222/webrev.01/index.html
Yes and I think should also rethrow the cause if the cause is an Error (why not) Mandy
On Dec 1, 2017, at 5:17 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Dec 1, 2017, at 4:46 PM, Brent Christian <brent.christian@oracle.com> wrote:
On 12/1/17 11:40 AM, Brent Christian wrote: On 12/1/17 8:33 AM, mandy chung wrote:
Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with.
Yes that would be better. So would I do this for RuntimeException and Error, and then package checked types of causes in an Error, as Mandy suggested? (We don't want to throw a checked exception from initSystemClassLoader(), I don't think.)
Anyway, this is what that looks like (I did RuntimeException, but didn't bother with Error). I also made the test changes that Mandy suggested.
http://cr.openjdk.java.net/~bchristi/8187222/webrev.01/index.html
Yes and I think should also rethrow the cause if the cause is an Error (why not)
The instanceof RuntimeException check should be moved outside to the if-statement when it’s an instance of InvocationTargetException. Mandy
On 12/1/17 5:22 PM, Mandy Chung wrote:
I think should also rethrow the cause if the cause is an Error (why not)
Alright.
The instanceof RuntimeException check should be moved outside to the if-statement when it’s an instance of InvocationTargetException.
So rethrow RuntimeExceptions directly, whether they're the cause of an InvocationTargetException or not. Updated webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.02/ Thanks, -Brent
On 12/4/17 12:11 PM, Brent Christian wrote:
So rethrow RuntimeExceptions directly, whether they're the cause of an InvocationTargetException or not.
Updated webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.02/
Looks good. Thanks Mandy
On 04/12/2017 20:18, mandy chung wrote:
On 12/4/17 12:11 PM, Brent Christian wrote:
So rethrow RuntimeExceptions directly, whether they're the cause of an InvocationTargetException or not.
Updated webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.02/
Looks good.
Yes, I think this looks okay (although future maintainers might wonder about the additionally unwrapping). -Alan
participants (3)
-
Alan Bateman
-
Brent Christian
-
mandy chung