RFR: 8306471: Add virtual threads support to JDWP ThreadReference.Stop and JDI ThreadReference.stop() [v11]

[Alan Bateman]

On Fri, 12 May 2023 03:54:46 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) (#13546) added some virtual thread support to JVMTI StopThread. This allows JDWP ThreadReference.Stop and JDI ThreadReference.stop() to have the same level support for virtual threads.
>> 
>> For your reference, here are the JVMTI spec changes that this pr is using as the basis for the JDWP and JDI spec updates.
>> 
>> https://github.com/openjdk/jdk/pull/13546/files#diff-cab9797fde25f344784e4473778cd35bfd9e19ba36f82c11b1a0674636958339
>> 
>> Mostly this is a spec update for JDWP and JDI, but there are also some minor changes needed to the ThreadReference.stop() handling of JDWP errors, and throwing the appropriate exceptions. Also some minor cleanup in jdb. The debug agent doesn't need changes since JVMTI errors are just passed through as the corresponding JDWP errors, and the code for this is already in place. These errors are not new nor need any special handling.
>> 
>> Our existing testing for ThreadReference.stop() is fairly weak:
>> 
>> - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a breakpoint. It will get problem listed by [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for it already working and will push it separately.
>> - nsk/jdi/stop/stop001, which is problem listed and only tests when the thread is blocked in Object.wait()
>> - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception fails properly
>> 
>> I decided to take stop002 and make it a more thorough test of ThreadReference.stop(). See the comment at the top of the test for a list of what is tested. As for reviewing this test, it's probably best to look at it as a completely new test rather than looking at diffs since so much has been changed and added.
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix error message to print correct exception name

I've looked at the spec + implementation changes, didn't have time to study the tests closely. Only comment is "current frame".

src/java.se/share/data/jdwp/jdwp.spec line 2025:

> 2023:             (Error OPAQUE_FRAME   "The thread is a suspended virtual thread and the implementation "
> 2024:                                   "was unable to throw an asynchronous exception "
> 2025:                                   "from the current frame.")

This is okay but could be a bit clearer with "from the thread's current frame".

src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 132:

> 130:      * @throws OpaqueFrameException if the thread is a suspended
> 131:      * virtual thread and the implementation was unable to throw an
> 132:      * asynchronous exception from the current frame

I think "from the thread's current frame" might be a bit clearer here, more so than the JDWP spec because the current frame could be read as the current JDI (if you see what I mean). What you have is okay too, but like the JDWP spec, could be a bit clearer.

-------------

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13548#pullrequestreview-1423860540
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1191964147
PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1191965097