On Fri, 28 Oct 2022 01:32:50 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 1684: >> >>> 1682: } >>> 1683: debugMonitorExit(callbackLock); >>> 1684: waiting_for_active_callbacks = JNI_FALSE; >> >> The set operations at lines 1678 and 1684 are not protected by any lock. >> Would it be more safe to put them inside critical section at lines 1679-1683? >> The variable `waiting_for_active_callbacks` is checked by the event >> callbacks. > > Only the "JDWP Command Reader" thread can ever enter this code. So we have > one writer but multiple readers. The readers all have to grab the > callbackLock before reading `waiting_for_active_callbacks`, and also before > touching `active_callbacks`. It is possible that the flag can be set and then > `END_CALLBACK()` does the notification before the above code enters the lock > and waits. But that's ok because it also means that `active_callbacks` is set > to 0, so no wait will be done. If another event thread races and sets it back > up to 1, then that is also ok because that means END_CALLBACK() will be > invoked again and do the notification again. > > However, there also seems to be no reason not to move the code withing the > critical section, so I can do that if you'd like. The reason it ended up > outside is because I used to have a different flag (I think it was called > `gdata->resetting`) that was set in `debugInit_reset()` (a couple of frames > above this one), but I decided it would be easier to understand if I limited > the scope of when the flag was set, and also give it a name that implied a > narrower scope of use. However, I maintained the same semantics of setting it > outside the lock. Thank you for explanation. Moving it inside critical section makes it more safe and clean. ------------- PR: https://git.openjdk.org/jdk/pull/10865
