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

[serguei.spitsyn at oracle.com]

On 2/19/14 7: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)
>

It looks good to me.
Thank you for changes in the fix!

Thanks,
Serguei


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