[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Integrated: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not hav
From:       Stefan Karlsson <stefank () openjdk ! org>
Date:       2023-11-30 9:49:40
Message-ID: 4KIVsDOaiNSWSjgyJSd8X05mwzTQqsUryyPA3qJbr1A=.10ce1064-bc8d-45f7-83f9-a0a6f1c53b6d () github ! com
[Download RAW message or body]

On Wed, 22 Nov 2023 15:00:29 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:

> In the rewrites made for:
> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump asserts \
> in interleaved ObjectMonitor::deflate_monitor calls` 
> I removed the filtering of *owned ObjectMonitors with dead objects*. The reasoning \
> was that you should never have an owned ObjectMonitor with a dead object. I added \
> an assert to check this assumption. It turns out that the assumption was wrong *if* \
> you use JNI to call MonitorEnter and then remove all references to the locked \
> object. 
> The provided tests provoke this assert form:
> * the JNI thread detach code
> * thread dumping with locked monitors, and
> * the JVMTI GetOwnedMonitorInfo API.
> 
> While investigating this we've found that the thread detach code becomes more \
> correct when this filter was removed. Previously, the locked monitors never got \
> unlocked because the ObjectMonitor iterator never exposed these monitors to the JNI \
> detach code that unlocks the thread's monitors. That bug caused an ObjectMonitor \
> leak. So, for this case I'm leaving these ObjectMonitors unfiltered so that we \
> don't reintroduce the leak. 
> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so I'm \
> filtering those in the closure that collects ObjectMonitor. Side note: We have \
> discussions about ways to completely rewrite this by letting each thread have \
> thread-local information about JNI held locks. If we have this we could probably \
> throw away the entire ObjectMonitorDump hashtable, and its walk of the \
> `_in_use_list.`. 
> For GetOwnedMonitorInfo it is unclear if we should expose these weird \
> ObjectMonitor. If we do, then the users can detect that a thread holds a lock with \
> a dead object, and the code will return NULL as one of the "owned monitors" \
> returned. I don't think that's a good idea, so I'm filtering out these \
> ObjectMonitor for those calls. 
> Test: the written tests with and without the fix. Tier1-Tier3, so far.

This pull request has now been integrated.

Changeset: 0d146361
Author:    Stefan Karlsson <stefank@openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/0d146361f27e1415fab9272de1cdde84c074c718
                
Stats:     326 lines in 8 files changed: 319 ins; 1 del; 6 mod

8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not \
have a dead object

Reviewed-by: dholmes, ihse, sspitsyn, dcubed

-------------

PR: https://git.openjdk.org/jdk/pull/16783


[prev in list] [next in list] [prev in thread] [next in thread]