RFR JDK-8198253: ThreadInfo.from(CompositeData) assigning fields incorrectly in JDK 9

[mandy chung]

On 2/20/18 9:48 PM, Jeremy Manson wrote:
> I think that's a much better approach (I didn't notice the validate 
> method :) )
>
> I think you may want to grab my test changes, or make some similar 
> ones.  Presumably, the tests do not pass with your change.

I ran your test and it passed before posting it.
>
> I think the API wording change is very slightly confusing.
>
>         *       A {@code CompositeData} does not contain this attribute
>           *       when representing a {@code ThreadInfo} of JDK 6 or
>     older version.
>           *       This attribute will be set to {@link
>     Thread#NORM_PRIORITY}.</td>
>
>
> I'd probably say "In such cases" at the beginning of the second 
> sentence.  Also, is there a rule about when you say JDK 6 and when you 
> say Java 6?

Java SE 6 should be the proper terminology be used in this case.  I 
actually try to see if we can use @since instead.  I will take any pass 
on the spec and how it can tie with @since in the getter methods.

I will look at it later today.

Mandy
>
> Jeremy
>
> On Tue, Feb 20, 2018 at 6:41 PM, mandy chung <mandy.chung at oracle.com 
> <mailto:mandy.chung at oracle.com>> wrote:
>
>     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/
>     <http://cr.openjdk.java.net/%7Emchung/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.chung at oracle.com <mailto:mandy.chung at 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
>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180221/a92ef0c7/attachment.html>