On Thu, 21 Oct 2021 08:01:30 GMT, Richard Reingruber <rr...@openjdk.org> wrote:
>> BTW, normally I would not suggest less locking simply because I *thought* it >> might not be needed. However, you already temporarily release and then >> regrab the lock. This means there is a period of time where the locking is >> not needed. What I've suggested is to better define (and expand) that period >> of time so the locking can be made cleaner. I think the actual expansion of >> time where the lock would no longer be held is pretty small, with a well >> defined set of code being executed that to me does not appear to require the >> lock, and if it did require the lock, then probably releasing the lock in >> the first place is a bug. > >> move the locking and unlocking into doPendingTasks(). At the start of the >> node->handlingAppResume block, grab handlerLock and threadLock, and explain >> that handlerLock is being grabbed because trackAppResume() will (indirectly) >> try to grab it, and grabbing it before threadLock is necessary for lock >> ordering. Then grab threadLock and hold onto it until the end of the block. > > So threadLock would be released at the end of the new block that depends on > node->handlingAppResume. After that block follow accesses to > `pendingInterrupt` > and `pendingStop` that would be unsynchronized then. I think they do need to > be > synchronized through threadLock though. If you are worried about another thread changing those fields after they have already been checked, then you can use the threadLock around them also. So you can grab threadLock before the `if (node->handlingAppResume)` and release after the `pendingInterrupt` and `pendingStop` references. I don't think it's really needed, because this is the only place where the flags are set false (and some action is taken when true), but there is no harm in holding the threadLock here, and it doesn't make the locking any more complicated. ------------- PR: https://git.openjdk.java.net/jdk/pull/5849
