On Fri, 12 Jul 2024 04:08:25 GMT, Chris Plummer <[email protected]> wrote:
>> test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/resume/resume001.java
>> line 382:
>>
>>> 380: if (expresult != returnCode0) {
>>> 381: vm.resume();
>>> 382: vm.resume(); // for case error when both
>>> VirtualMachine and the thread2 were suspended
>>
>> Pre-existing but I don't understand the comment. Why would you need 2
>> `vm.resume()` here? If `thread2` was suspended directly don't you need a
>> `thread2.resume()`?
>
> First just to clarify a general JDI feature about thread suspending and
> resuming. You can undo a ThreadReference.suspend() or a thread suspended as
> the result of an event by dong a vm.resume(). This is documented in the JDI
> API spec, which talks about suspendCounts and how various APIs and event
> delivery affect them.
>
> I was tempted to clean up these vm.resume() calls but opted not to. The
> point being made in the comment is that worse case thread2 was suspended by a
> breakpoint or thread2.suspend() and all threads were suspended by a
> vm.resume() (meaning thread2 could have a suspendCount of 2). Two
> vm.resumes() take are done to make sure thread2 gets resumed under this
> situation. However, one of the vm.resume calls could instead be a
> thread2.resume(). Doing two vm.resume() calls was probably just laziness by
> the original authors. It works though.
>
> However, by my accounting at any failure point thread2 never has a
> suspendCount > 1, so really just one vm.resume() would be enough.
>
> The original code did these two vm.resume() calls unconditionally, but they
> are not needed if there was no error.
The original code had 2 vm.resume() - one on them to match vm.suspend() and 2nd
one to allow debugee to continue on error.
Now we have 3 vm.resume() - one is to match vm.suspend() (line 377) and 2
conditional (on error).
Theoretically we can get an error when both vm and thread2 are suspended, so 2
vm.resume() looks reasonable.
Anyway resume() is a nop if the thread is not suspended
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1675474511
