'Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with Invocatio'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with Invocatio
From:       Jaroslav Bachorik <jaroslav.bachorik () oracle ! com>
Date:       2014-02-24 9:04:30
Message-ID: 530B0B1E.3060508 () oracle ! com
[Download RAW message or body]

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

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@oracle.com wrote:
>>>>> On 2/17/14 12:04 AM, Jaroslav Bachorik wrote:
>>>>>> On 14.2.2014 23:13, serguei.spitsyn@oracle.com wrote:
>>>>>>> On 2/14/14 12:33 PM, Daniel D. Daugherty wrote:
>>>>>>>> On 2/14/14 11:46 AM, serguei.spitsyn@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-
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic