On Tue, 12 Oct 2021 22:55:05 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2200: > >> 2198: /* trackAppResume() needs handlerLock */ >> 2199: debugMonitorExit(threadLock); >> 2200: eventHandler_lock(); /* for proper lock order */ > > Is it still necessary for the handlerLock to be held when calling > `blockOnDebuggerSuspend()` (presuming you don't also add the new handlerLock > related code in it)? It seems that only the threadLock is needed so it can > then wait on it. > > The main thing you've done to fix this issue is defer the > `blockOnDebuggerSuspend()` to be done after `event_callback()` has released > the handlerLock, and to make it so `blockOnDebuggerSuspend()` does not block > while holding the handlerLock, so is there really any reason to be grabbing > it at all? Please see my answer on your [comment for L2220](https://github.com/openjdk/jdk/pull/5849#discussion_r727573811) > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2220: > >> 2218: * suspends a thread it will remain suspended. >> 2219: */ >> 2220: trackAppResume(resumer); > > `trackAppResume()` doesn't really need the handlerLock. However, it will grab > it when calling `eventHandler_createInternalThreadOnly()`. Since you want to > make sure it is grabbed before threadLock in order to preserve lock ordering, > that complicates things if we decided not to grab the handlerLock in the code > above. What I'm now thinking is all that is really needed is to hold the > threadLock around the call to `blockOnDebuggerSuspend()`, or better yet just > grab it from within `blockOnDebuggerSuspend()`. You probably don't need to do > anything with handlerLock or threadLock inside of `doPendingTasks()`. After returning from `blockOnDebuggerSuspend()` we have to make sure resumee cannot be suspended by JDWP means otherwise the current thread which is about to execute j.l.Thread.resume() will interfere with the debugger. This is achieved by holding threadLock until the tracking is established by `trackAppResume()`. For symmetry the set of owned locks should be equal before/after calling `blockOnDebuggerSuspend()` I think. Therefore handlerLock and threadLock are acquired before. ------------- PR: https://git.openjdk.java.net/jdk/pull/5849
