RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

[serguei.spitsyn at oracle.com]

Hi David,


On 5/27/20 02:00, David Holmes wrote:
> On 27/05/2020 6:36 pm, serguei.spitsyn at oracle.com wrote:
>> Hi David,
>>
>>
>> On 5/27/20 00:47, David Holmes wrote:
>>> Hi Serguei,
>>>
>>> On 27/05/2020 1:01 pm, serguei.spitsyn at oracle.com wrote:
>>>> Please, review a fix for:
>>>>    https://bugs.openjdk.java.net/browse/JDK-8234882
>>>>
>>>> CSR draft (one CSR reviewer is needed before finalizing it):
>>>>    https://bugs.openjdk.java.net/browse/JDK-8245853
>>>
>>> I have some thoughts on the wording which I will add to the CSR.
>>
>> Thank you a lot for looking at this!
>>
>>> Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would the 
>>> best error to use, and it has an equivalent in JDWP and at the Java 
>>> level for JDI.
>>
>> This is an interesting variant, thanks!
>> We need to balance on several criteria:
>>   1) Compatibility: keep returning error as close as possible to the 
>> current spec
>
> If you are adding a new error condition I don't understand what you 
> mean by "close to the current spec" ??

If the JVMTI_ERROR_INVALID_OBJECT is returned than the JDWP agent does 
not need any new error handling.
The same can be true in the JDI if the JDWP returns the same error as it 
returned before.
In this case we do not add new error code but extend the existing to 
cover new error condition.

But, in fact (especially, after rethinking), I do not like the 
JVMTI_ERROR_INVALID_OBJECT
error code as it normally means something different.
So, let's avoid using it and skip this criteria.
Then we need new error code to cover new error condition.

>>   2) Best error naming match between JVM TI and JDI/JDWP
>>   3) Best practice in errors naming
>
> If the argument is not a ThreadDeath instance then it is an illegal 
> argument - perfect fit semantically all the specs involved have an 
> "illegal argument" error form.

I agree with this.
It is why I like this suggestion. :)
The JDWP equivalent is: ILLEGAL_ARGUMENT.
The JDI equivalent is:  IllegalArgumentException

I'll prepare and send the update.

Thanks!
Serguei


> Cheers,
> David
>
>> I think the #1 is most important but will look at it once more.
>>
>> Thanks,
>> Serguei
>>
>>> Thanks,
>>> David
>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 
>>>>
>>>>
>>>> Updated JVM TI StopThread spec:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>>>>
>>>>
>>>> Summary:
>>>>
>>>>    The JVM TI StopThread method mirrored the functionality of the
>>>>    java.lang.Thread::stop(Throwable t) method, in that it allows 
>>>> any exception
>>>>    type to be installed as an asynchronous exception in the target 
>>>> thread.
>>>>    However, the java.lang.Thread::stop(Throwable t) method was 
>>>> inherently unsafe
>>>>    and in Java 8 (under JDK-7059085) it was "retired" so that it 
>>>> always threw
>>>>    UnsupportedOperationException.
>>>>    The updated JVM TI StopThread spec disallows an arbitrary 
>>>> Throwable from being passed,
>>>>    and instead restricts the argument to being an instance of 
>>>> ThreadDeath, thus
>>>>    mirroring the (deprecated but still functional) 
>>>> java.lang.Thread::stop() method.
>>>>    The error JVMTI_ERROR_INVALID_OBJECT is returned if the 
>>>> exception argument
>>>>    is not an instance of ThreadDeath.
>>>>
>>>>    Also, I will file similar RFE and CSR on the JDI and JDWP spec.
>>>>
>>>>
>>>> Testing:
>>>>    Built docs and checked the doc has been generated as expected.
>>>>    Will run the nsk.jvmti tests locally.
>>>>    Will submit hs-tiers1-3 to make sure there are no regressions in 
>>>> the JVM TI and JDI tests.
>>>>
>>>> Thanks,
>>>> Serguei
>>