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

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a
From:       Stefan Karlsson <stefank () openjdk ! org>
Date:       2023-11-30 9:49:38
Message-ID: jorxSkiOWkRRDjqqCyKmKqQQSAyrLRV_IXVJNPltIc0=.11ed29e4-7265-4e09-9cbf-772b03e87124 () github ! com
[Download RAW message or body]

On Wed, 29 Nov 2023 06:38:51 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.
> 
> Stefan Karlsson has updated the pull request incrementally with one additional \
> commit since the last revision: 
> Review comments

Thanks all for reviewing!

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

PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1833418484


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