RFR(M): 8203826: Chain class initialization exceptions into later NoClassDefFoundErrors

[David Holmes]

On 4/07/2018 6:21 PM, Lindenmaier, Goetz wrote:
> Hi,
> 
> Volker, thanks for improving on my original change and
> implementing David's and Karen's proposals.
> 
> David, I think the change addresses a row of your concerns.
> 
>> proposal - it just moves the "field" from the Java side to the VM side.
> No, the space overhead in case of successful initialization is
> reduced from O(classes) to O(classloaders).

I don't think space usage was one of my concerns.

> There is only runtime overhead if there was an error, and
> that should be acceptable.
> 
>> dealing with backtrace and stackTrace. I have to wonder why nothing in
>> Throwable clears the backtrace today ?
> Maybe the concern about the backTraces is pointless and the
> conversion to stackTraces should be dropped.
> As you say, it's done nowhere else, and other backTraces should
> cause similar issues.

My comment on the clearing of Throwable.backtrace was purely in relation 
to the internal state protocol of Throwable, not about the conversion 
process as such.

The conversion process is needed, as previously discussed, to avoid 
class/classloader leaks. Exceptions are normally thread-contained so 
there is limited scope for creating class/classloader leaks. However as 
soon as you allow this original exception to be stored and potentially 
later accessed in a different thread then the leak is possible - it 
might even be possible to hit some kind of loader constraint error if a 
same named type can be loaded by different classloaders used by the 
different threads.

Add some extra information about the type of the original exception by 
all means, but not the stacktrace (unless you've stringified it). As I 
say the place the stack is of interest in when the original exception is 
encountered. If something is swallowing this then logging is the tool to 
expose it IMHO.

>> I'm not clear why you record the ExceptionInInitializerError wrapper
>> instead of the actual exception that occurred?
> Keeping the ExceptionInInitializerError is helpful for people
> to understand that this has happened during initialization and not directly
> where the NCDFE is thrown. They will understand that this might have
> happened in another thread.

The NCDFE  message already states something like "cannot initialize 
class because prior initialization attempt failed", which I think makes 
it quite clear.

I dispute "they will understand this might have happened in another thread".

Cheers,
David

> Best regards,
>    Goetz.
> 
> 
> 
> 
> 
> 
>> -----Original Message-----
>> From: core-libs-dev [mailto:core-libs-dev-bounces at openjdk.java.net] On
>> Behalf Of David Holmes
>> Sent: Sonntag, 1. Juli 2018 23:48
>> To: Volker Simonis <volker.simonis at gmail.com>; hotspot-runtime-
>> dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>;
>> Java Core Libs <core-libs-dev at openjdk.java.net>
>> Subject: Re: RFR(M): 8203826: Chain class initialization exceptions into later
>> NoClassDefFoundErrors
>>
>> Hi Volker,
>>
>> This doesn't really address any of the concerns I had with the original
>> proposal - it just moves the "field" from the Java side to the VM side.
>> There is still a massive amount of Java code execution in relation to
>> this - which itself may encounter secondary exceptions. It's very hard
>> to tell if you will leave things in a suitable state if such exceptions
>> arise.
>>
>> My position remains that the primary place to deal with the
>> initialization error is when initialization occurs and the error
>> happens. Subsequent attempted uses of the erroneous class may benefit
>> from some additional information about the nature of the original
>> exceptions, but I don't think full stacktraces are necessary or
>> desirable (and I do believe they will confuse most users given the lack
>> of continuity in the stack frames and that they may have happened in a
>> different thread!).
>>
>> That aside ...
>>
>> There appears to a race on constructing the Hashtable. At least it was
>> not obvious to me where a lock may be held during that process.
>>
>> I can't determine that clearing backtrace in removeNativeBacktrace is
>> correct with respect to the overall protocol within Throwable for
>> dealing with backtrace and stackTrace. I have to wonder why nothing in
>> Throwable clears the backtrace today ?
>>
>> I'm not clear why you record the ExceptionInInitializerError wrapper
>> instead of the actual exception that occurred?
>>
>> Throwable states:
>>
>> +      * This method is currently only called from the VM for instances of
>> +      * ExceptionInInitializerError which are stored for later chaining
>> into a
>> +      * NoClassDefFoundError in order to prevent keeping classes from
>> the native
>> +      * backtrace alive.
>> +      */
>>
>> but IIUC it will also be called for instances of Error that occurred
>> which do not get wrapped in EIIE.
>>
>>
>> Regards,
>> David
>> ------
>>
>>
>> On 30/06/2018 12:53 AM, Volker Simonis wrote:
>>> Hi,
>>>
>>> can I please have a review for the following change which saves
>>> ExceptionInInitializerError thrown during class initialization and
>>> chains them as cause into potential NoClassDefFoundErrors for the same
>>> class. We are using this features since years in our commercial SAP
>>> JVM and it proved extremely useful for detecting and fixing errors
>>> especially in big deployments.
>>>
>>> This is a follow-up on a discussion previously started by Goetz [1].
>>> His first proposal (which is close to our current, internal
>>> implementation) inserted an additional field into java.lang.Class
>>> objects to save potential ExceptionInInitializerErrors. This was
>>> considered to much overhead in the initial discussion [1].
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8203826.v2/
>>> https://bugs.openjdk.java.net/browse/JDK-8203826
>>>
>>> So in this change, I've completely re-implemented the feature by using
>>> a java.lang.Hashtable which is attached to the ClassLoaderData object.
>>> The Hashtable is lazily created when the first
>>> ExceptionInInitializerError is thrown and maps the Class which
>>> triggered the ExceptionInInitializerError during the execution of its
>>> static initializer to the corresponding ExceptionInInitializerError.
>>>
>>> If the same class will be accessed once again, this will directly lead
>>> to a plain NoClassDefFoundError (as per the JVMS, 5.5 Initialization)
>>> because the static initializer won't be executed a second time. Until
>>> now, this NoClassDefFoundError wasn't linked in any way to the root
>>> cause of the problem (i.e. the first ExceptionInInitializerError
>>> together with the chained exception that happened during the execution
>>> of the static initializer). With this change, the NoClassDefFoundError
>>> will now chain the initial ExceptionInInitializerError as cause,
>>> making it much easier to detect the problem which lead to the
>>> NoClassDefFoundError.
>>>
>>> Following is an example from the new JTreg tests which comes which
>>> this change to demonstrate the feature. Until know, a typical stack
>>> trace from a NoClassDefFoundError looked as follows:
>>>
>>> java.lang.NoClassDefFoundError: Could not initialize class
>>> NoClassDefFound$ClassWithFailedInitializer
>>>       at java.base/java.lang.Class.forName0(Native Method)
>>>       at java.base/java.lang.Class.forName(Class.java:291)
>>>       at NoClassDefFound.main(NoClassDefFound.java:38)
>>>
>>> With this change, the same stack trace now looks as follows:
>>>
>>> java.lang.NoClassDefFoundError: Could not initialize class
>>> NoClassDefFound$ClassWithFailedInitializer
>>>       at java.base/java.lang.Class.forName0(Native Method)
>>>       at java.base/java.lang.Class.forName(Class.java:315)
>>>       at NoClassDefFound.main(NoClassDefFound.java:38)
>>> Caused by: java.lang.ExceptionInInitializerError
>>>       at
>> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(
>> Native
>>> Method)
>>>       at
>> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(
>> NativeConstructorAccessorImpl.java:62)
>>>       at
>> java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstan
>> ce(DelegatingConstructorAccessorImpl.java:45)
>>>       at
>> java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
>>>       at java.base/java.lang.Class.newInstance(Class.java:584)
>>>       at
>> NoClassDefFound$ClassWithFailedInitializer.<clinit>(NoClassDefFound.java:2
>> 0)
>>>       at java.base/java.lang.Class.forName0(Native Method)
>>>       at java.base/java.lang.Class.forName(Class.java:315)
>>>       at NoClassDefFound.main(NoClassDefFound.java:30)
>>> Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 2 out of
>>> bounds for length 1
>>>       at NoClassDefFound$A.<clinit>(NoClassDefFound.java:9)
>>>       ... 9 more
>>>
>>> As you can see, the reason for the NoClassDefFoundError when accessing
>>> the class 'NoClassDefFound$ClassWithFailedInitializer' is actually not
>>> even in the class or its static initializer itself, but in the class
>>> 'NoClassDefFound$A' which is a base class of
>>> 'NoClassDefFound$ClassWithFailedInitializer'. This is not easily
>>> detectible from the old, plain NoClassDefFoundError.
>>>
>>> As I wrote, the only overhead we have with the new implementation is
>>> an additional OopHandle field per ClassLoaderData which I think is
>>> acceptable. The Hashtable object itself is only created lazily, after
>>> the first occurrence of an ExceptionInInitializerError in the
>>> corresponding class loader. The whole Hashtable creation and
>>> storing/quering of ExceptionInInitializerErrors in
>>>
>> ClassLoaderData::record_init_exception()/ClassLoaderData::query_init_exce
>> ption()
>>> is optional in the sense that any errors/exceptions occurring during
>>> the execution of these functions are ignored and cleared.
>>>
>>> Finally, we also take care to recursively convert all native
>>> backtraces in the stored ExceptionInInitializerErrors (and their
>>> suppressed and chained exceptions) into symbolic stack traces in order
>>> to avoid holding references to classes and prevent their unloading.
>>> This is implemented in the new private, static method
>>> java.lang.Throwable::removeNativeBacktrace() which is called for each
>>> ExceptionInInitializerError before it is stored in the Hashtable.
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>> [1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-
>> June/028310.html
>>>