Hi Serguei,
On 7/10/2019 6:42 pm, [email protected] wrote:
Hi David,
Suppose, T1 thread is blocked on a raw monitor hold by thread T3 in JVMTI
MonitorContendedEntered callback on an object monitor hold by thread T2.
In theory, if only raw monitor is used in further deadlock detection
analysis
then a possible dependency loop with T1 an T2 involved won't be detected.
For full analysis the dependency chain through object monitor have to be
also analyzed.
This would add extra complexity into the deadlock detection code for
very rare corner case.
I do not care much about this, and so, like the approach.
To be clear the case you describe is not handled by either the current
code nor my proposed update.
The fix itself looks good to me.
Thanks.
David
Thanks,
Serguei
On 10/3/19 20:58, David Holmes wrote:
Okay, to allow for me to make forward progress here I've decided to
follow the "principle of least brokenness" ;-)
Recap: Because of JVMTI event callbacks it is possible for a thread to
set its current pending monitor as a JvmtiRawMonitor when it was
already set for an ObjectMonitor. This is broken in at least two ways:
- when the raw monitor use completes the pending monitor is set to
NULL, not restored to the ObjectMonitor
- whilst the raw monitor is seen as the pending monitor the
ObjectMonitor is not considered by the deadlock detection logic
Ignoring what I'm currently doing with jvmtiRawMonitor, I do not know
how to fix the above brokenness and it is out of scope for what I am
trying to do.
So what I propose is to make things no more broken than they are now,
and actually improve things a little:
- the pending JvmtiRawMonitor is given preference over the
ObjectMonitor in the deadlock detection code (this emulates current
situation of the raw monitor overwriting the pending ObjectMonitor)
- we no longer lose the fact we were also pending on an ObjectMonitor
- the stack printing code in the deadlock detector prints information
about both the raw monitor and the ObjectMonitor
Updated webrev:
http://cr.openjdk.java.net/~dholmes/8231289/webrev.v2/
The only change is threadService.cpp
Thanks,
David
