On Fri, 12 May 2023 03:54:46 GMT, Chris Plummer <cjplum...@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
