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

[Jeremy Manson]

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 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?

Jeremy

On Tue, Feb 20, 2018 at 6:41 PM, mandy chung <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/
>
> 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/
>
> On Fri, Feb 16, 2018 at 6:20 PM, mandy chung <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/
>>
>> Bug:
>> 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/20180220/611f1264/attachment-0001.html>