On Fri, 17 Nov 2023 21:19:05 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: add comment for new ThreadsListHandle use > > src/hotspot/share/prims/jvmtiEventController.cpp line 374: > >> 372: // Protects any existing JavaThread's from being terminated while it >> is set. >> 373: // The FJP carrier thread compensating mechanism can create carrier >> threads concurrently. >> 374: ThreadsListHandle tlh(current); > > A ThreadsListHandle does not prevent a JavaThread from being terminated. It > prevents a JavaThread from exiting and being freed. The JavaThread is able to > set the terminated state on itself, but will not be able to complete exiting > while > it is on a ThreadsListHandle. There is a subtle difference. > > There's a `target` JavaThread that is fetched from a `JvmtiThreadState` object > and that `target` JavaThread is only protected by this `tlh` if `target` is > included > in the ThreadsList that was captured by this `tlh`. In all likelihood, there > should be > a ThreadsListHandle farther up the stack that's protecting the JavaThread from > which the `JvmtiThreadState` object was extracted and passed to this function. > > As for carrier threads, if they are created _after_ this `tlh` was created, > then this > `tlh` cannot protect them because they won't be on this `tlh`'s ThreadsList. Thank you for the comment, Dan! Agreed, the comment needs to be corrected in two aspects. I tried to simplify it but failed to do it correctly. It is interesting that there is a `ThreadsListHandle` farther up the stack but it does not help sometimes. It is observed that a `JavaThread` (of a carrier thread) referenced from the `JvmtiThreadState` object can be just created, so we need a `ThreadsListHandle` to avoid possible asserts. With this fix in place I do not see the asserts fired anymore. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1398200015
