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.newInstance(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:20)
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_exception()
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
