RFR 4505697: nsk/jdi/ExceptionEvent/itself/exevent006 and exevent008 tests fail with InvocationTargetException (original) (raw)

Jaroslav Bachorik jaroslav.bachorik at oracle.com
Mon Feb 24 01:04:30 PST 2014

On 21.2.2014 08:24, David Holmes wrote:

On 20/02/2014 11:41 PM, Jaroslav Bachorik wrote:

On 20.2.2014 11:40, David Holmes wrote:

Hi Jaroslav,

instanceKlass.cpp: Comment is wrong: 913 // JVMTI internal flag reset is needed in order to report InvocationTargetException It will be ExceptionInInitializerError Will fix. Copypaste ...

You added this: 917 thisoop->setinitializationstateandnotify(initializationerror, THREAD); 918 CLEARPENDINGEXCEPTION; // ignore any exception thrown, class initialization error is thrown below + 919 // JVMTI has already reported the pending exception + 920 // JVMTI internal flag reset is needed in order to report InvocationTargetException + 921 JvmtiExport::cleardetectedexception((JavaThread*)THREAD); but there are a number of places where setinitializationstateandnotify is called when a pending exception has been cleared, and then CLEARPENDINGEXCEPTION is called again, but you didn't modify those other locations. They will rethrow the original exception so I suppose that is okay from JVMTI's perspective. But the flip-side of this is that if setinitializationstateandnotify does throw an exception, JVMTI will never see it. I don't know if it supposed to see it. It seems that any exception thrown from setinitializationstateandnotify is thoroughly ignored and hidden from the outer world. Perhaps someone more experienced in JVMTI than me would like to chime in here? Serguei? setinitializationstateandnotify is often called when an exception has already occurred during the class loading/initialization process. It is that original exception that we want to propagate but meanwhile we have to perform this action to update the state and wakeup any waiters. So we cache the original exception, clear it, do the state update and then clear any pending exception (I think the only exception possible here is OOME!), then rethrow the original. If this action did indeed throw OOME then we might not be able to wake up the waiter(s) and that might lead to a hang. While a debug VM could use TraceExceptions to (hopefully) spot the OOME, in a product VM it would be invisible, even if a JVMTI agent was tracking exceptions. So I think it should be visible to JVMTI. I would like to hear other opinions though. However this is going beyond the scope of fixing these particular tests so I'm fine if this is simply recorded in another bug for future clean up.

Created https://bugs.openjdk.java.net/browse/JDK-8035646

Thanks for the review, everyone.

-JB-

Thanks, David

-JB-

--- jvm.cpp Comment is wrong again - not InvocationTargetException. --- David ------

On 20/02/2014 1:59 AM, Jaroslav Bachorik wrote: On 18.2.2014 11:18, serguei.spitsyn at oracle.com wrote: On 2/17/14 12:04 AM, Jaroslav Bachorik wrote: On 14.2.2014 23:13, serguei.spitsyn at oracle.com wrote: On 2/14/14 12:33 PM, Daniel D. Daugherty wrote: On 2/14/14 11:46 AM, serguei.spitsyn at oracle.com wrote: Jaroslav,

It looks good in general modulo indent comments from Dan. But I have a doubt that acquiring the JvmtiThreadStatelock is needed or right thing to do in the JvmtiExport::cleardetectedexception(). It seems, both clearexceptiondetected() and setexceptiondetected() are always called on current thread and so, it has to be safe to do without acquiring any locks. My JVM/TI-foo is rusty, but I believe that JvmtiThreadState stuff can also be queried/modified by other threads so grabbing the associated lock is a good idea. The lock synchronization is cooperative. It does not help much if the lock is not acquired in other places. I can be wrong, but I've not found yet any place in the code where the clearexceptiondetected() and setexceptiondetected() are called under protection of the JvmtiThreadStatelock. I copied the locking over from "JvmtiExport::cleanupthread(JavaThread* thread)". That method is also supposed to work only with the current thread but acquires the lock nonetheless. But if you are sure that the lock is not required I have no objections removing it. I'm suggesting to remove it, as it is not used in other places in the code. It is going to be confusing if it is used in one place and missed in others. I've removed the lock and applied the same cleanup logic to other places where exceptions are rewrapped. Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.02 JPRT run: http://prt-web.us.oracle.com//archive/2014/02/2014-02-19-114618.jbachorik.hotspot/

Aurora Adhoc: http://aurora.ru.oracle.com//faces/Batch.xhtml?batchName=418853.VMSQE.adhoc.JPRT.full (still running at the moment; no failures so far) Thanks, Serguei -JB- Thanks, Serguei

Dan

And I'm repeating my question about pre-integration testing (Dan is asking about the same). Thanks, Serguei On 2/14/14 3:07 AM, Jaroslav Bachorik wrote: This is a round-0 review request. The reflection code intercepting the exceptions thrown in the invoked methods does not play nicely with JVMTI (which, in this case, propagates to JDI). The reflection code lacks the traditional error handler - therefore, upon throwing the NumberFormatException, the stack is searched for appropriate handlers and none are found. This leaves the "exceptiondetected" flag set to true while normally it would be reset to false once the exception is handled. The reflection code then goes on and wraps the NumberFormatException into InvocationTargetException and throws it. But, alas, the "exceptiondetected" flag is still set to true and no JVMTI exception event will be sent out. The proposed solution is to call thread->jvmtithreadstate()->clearexceptiondetected() at the appropriate places in the reflection code to reset the "exceptiondetected" flag and enable the InvocationTargetException be properly reported over JVMTI. Issue : https://bugs.openjdk.java.net/browse/JDK-4505697 Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00 Thanks! -JB-

More information about the serviceability-dev mailing list