On Wed, 13 Jul 2022 13:20:45 GMT, Zhengyu Gu <z...@openjdk.org> wrote:
>> Ok. Everything is passing after your latest fix. >> >> I think the test still has a synchronization bug, but probably not something >> important enough to worry about, and I'm guessing a lot of tests have this >> issue. The test disables events and then frees the raw monitor used in the >> callback. The problem is that JVMTI might already be in the process of >> calling the callback, so the callback will still attempt to use the monitor. >> The test shutdown code and the test callback code need to be synchronized to >> avoid this issue. However, there is a chicken and egg problem here, because >> you need a raw monitor for the synchronization. I think the real solution is >> to just not bother freeing up the raw monitor. I think we may have done this >> recently in some other test. Perhaps @sspitsyn remembers. > >> Ok. Everything is passing after your latest fix. >> >> I think the test still has a synchronization bug, but probably not something >> important enough to worry about, and I'm guessing a lot of tests have this >> issue. The test disables events and then frees the raw monitor used in the >> callback. The problem is that JVMTI might already be in the process of >> calling the callback, so the callback will still attempt to use the monitor. >> The test shutdown code and the test callback code need to be synchronized to >> avoid this issue. However, there is a chicken and egg problem here, because >> you need a raw monitor for the synchronization. I think the real solution is >> to just not bother freeing up the raw monitor. I think we may have done this >> recently in some other test. Perhaps @sspitsyn remembers. > > Me too have a concern on synchronization for enabling/disabling a JVMTI event: > While setting event bits is synchronized with `JvmtiThreadState_lock` in > [JvmtiEventControllerPrivate::set_user_enabled()](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEventController.cpp#L889), > but reading > [`is_enabled()](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnvBase.hpp#L308) > is unsynchronized. > > I don't know how it supposes to work, but at least > [`_enabled_bits`](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEventController.hpp#L85) > should be an `volatile`? @zhengyu123 I'll be OOO until tuesday, so after this evening I won't be able to respond. @sspitsyn will look at the JVMTI changes again and also help with some of your synchronization questions. ------------- PR: https://git.openjdk.org/jdk/pull/9168
