RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Brent Christian-2
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Mandy Chung


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Alan Bateman
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Mandy Chung


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Brent Christian-2
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Brent Christian-2
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Mandy Chung

> On Dec 1, 2017, at 4:46 PM, Brent Christian <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Mandy Chung


> On Dec 1, 2017, at 5:17 PM, Mandy Chung <[hidden email]> wrote:
>
>
>>> On Dec 1, 2017, at 4:46 PM, Brent Christian <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Brent Christian-2
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Mandy Chung


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Alan Bateman


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