Hi Jeremy,
Another approach is to change ThreadInfoCompositeData::validateCompositeData
to validate the given CompositeData. I also revised
ThreadInfoCompositeData to
return the default value of the attributes, if not present.
This is the patch:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.00/
What do you think?
Mandy
On 2/20/18 4:46 PM, Jeremy Manson wrote:
(I dropped serviceability-dev from this thread by mistake! Oops!)
Okay. Here's the revised patch. LMK if that's what you had in mind.
http://cr.openjdk.java.net/~jmanson/8198253/webrev.01/
<http://cr.openjdk.java.net/%7Ejmanson/8198253/webrev.01/>
On Fri, Feb 16, 2018 at 6:20 PM, mandy chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>> wrote:
Hi Jeremy,
lockedMonitors and lockedSynchronizers attribute are not optional if that's
the issue you try to resolve. I think the specification should be clarified.
ThreadInfo::from supports the three different versions for interoperability:
1. CompositeData for JDK 1.5 ThreadInfo with no lockedMonitors and
lockedSynchronizers attribute
2. CompositeData for JDK 6 ThreadInfo with lockedMonitors and
lockedSynchronizers
attributes
3. CompositeData for JDK 9 ThreadInfo with lockedMonitors and
lockedSynchronizers
attributes and with daemon and priority attribute.
JMX client can connect to a running VM in any version and get back a
proper ThreadInfo.
If ThreadInfo::from is called with a CompositeData containing daemon and
priority attribute but lockedMonitors and lockedSynchronizers attributes are
absent then the given CompositeData is invalid and IllegalArgumentException
should be thrown.
Does this help?
Mandy
On 2/15/18 4:46 PM, Jeremy Manson wrote:
Hi folks,
Been a long time! I'd like someone to do a code review. This is
in code I wrote a few years ago, and got wrong. At the time,
David Holmes, Staffan Larsen, and Mandy Chung reviewed it. It
does mean that people using ThreadInfo.from(CompositeData) may be
getting the wrong values out for ThreadInfo, so it is definitely
worth fixing.
The patch below fixes the bug, but is a pretty questionable
approach. Better approaches happily considered.
Patch:
http://cr.openjdk.java.net/~jmanson/8198253/webrev.00/
<http://cr.openjdk.java.net/%7Ejmanson/8198253/webrev.00/>
Bug:
https://bugs.openjdk.java.net/browse/JDK-8198253
<https://bugs.openjdk.java.net/browse/JDK-8198253>
Thanks!
Jeremy
