RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

Thu Feb 20 23:24:57 PST 2014

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
>> this_oop->set_initialization_state_and_notify(initialization_error,
>> THREAD);
>>    918       CLEAR_PENDING_EXCEPTION;   // 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::clear_detected_exception((JavaThread*)THREAD);
>>
>> but there are a number of places where
>> set_initialization_state_and_notify is called when a pending exception
>> has been cleared, and then CLEAR_PENDING_EXCEPTION 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 set_initialization_state_and_notify 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 set_initialization_state_and_notify is thoroughly ignored
> and hidden from the outer world. Perhaps someone more experienced in
> JVMTI than me would like to chime in here? Serguei?

set_initialization_state_and_notify 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.

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 JvmtiThreadState_lock is
>>>>>>>> needed
>>>>>>>> or right thing to do in the
>>>>>>>> JvmtiExport::clear_detected_exception().
>>>>>>>> It seems, both clear_exception_detected() and
>>>>>>>> set_exception_detected() 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
>>>>>> clear_exception_detected() and set_exception_detected() are called
>>>>>> under protection of the JvmtiThreadState_lock.
>>>>>
>>>>> I copied the locking over from
>>>>> "JvmtiExport::cleanup_thread(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
>>>>>>>>> "exception_detected" 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
>>>>>>>>> "exception_detected" flag is still set to true and no JVMTI
>>>>>>>>> exception event will be sent out.
>>>>>>>>>
>>>>>>>>> The proposed solution is to call
>>>>>>>>> thread->jvmti_thread_state()->clear_exception_detected() at the
>>>>>>>>> appropriate places in the reflection code to reset the
>>>>>>>>> "exception_detected" 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-
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>