On Fri, 12 Jul 2024 02:44:28 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix copyright and jcheck error
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1675243246
