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

[Jeremy Manson]

On Mon, Feb 26, 2018 at 4:53 PM, mandy chung <mandy.chung at oracle.com> wrote:

>
>
> On 2/26/18 4:23 PM, Jeremy Manson wrote:
>
> Hi Mandy,
>
> Thanks for taking this on!  I'm happy to see that you are happy to do
> cleanups I was too timid to do (like adding the Factory in the tests).
>
> I note a few places in the test code where static initializers can throw
> RuntimeExceptions.  When I ran the tests, and a static initializer threw a
> RuntimeException, I didn't see it reflected in the output, so I had to add
> print statements.  Was I just doing it wrong, or is that a feature of jtreg?
>
>
> Do you see ExceptionInInitializerError and its cause should be
> RuntimeException?  ExceptionInInitializerError is thrown when <clinit>
> fails.
>

There was no relevant output.


>
>
>
> Just a couple of random nits:
>
> >  267      * @throws IllegalArgumentException if the given CompositeData
> does not
> >  268      * contain all attributes representing ThreadInfo of any
> release.
>
> Could this be misinterpreted?  If you have everything in V5, and only one
> of the V6 attributes, it contains all of the attributes representing the
> ThreadInfo of V5.  Maybe you want to say "does not contain exactly the
> attributes present in ThreadInfo for a particular release"?
>
>
> The javadoc has this statement:
>    A CompositeData representing a ThreadInfo of version N must contain all
> the attributes defined since N or earlier unless specified otherwise.
>
> What about:
> @throws IAE if the given CompositeData does not contain all {@linkplain
> #attrs ThreadInfo attributes} for a  release.
>
> I will add a link to the table.
>

That phrasing has the same problem.  It needs to be *exactly* the
ThreadInfo attributes for a release.


>
>
> For signatures like:
>
> > static CompositeType initV5CompositeType(CompositeType ctype) {
>
> I probably would change the name of ctype to indicate when you are reading
> the method that the parameter is always supposed to be the the
> CompositeType for ThreadInfo.
>
>
>
> Let me think about it and send another webrev.  I am tempted to improve
> and provide an automated way for the versioning support.
>
> Mandy
>
>
>
> On Fri, Feb 23, 2018 at 4:50 PM, mandy chung <mandy.chung at oracle.com>
> wrote:
>
>> Webrev at:
>>   http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.01
>>
>> This patch updates the spec to clarify what attributes are required
>> since which release.  There is a spec bug that "classLoaderName"
>> added in JDK 9 is missing in the CompositeData for StackTraceElement
>> but the implementation is correct.  I will file a CSR for this update.
>>
>> This patch ensures that CompositeData for ThreadInfo of version N
>> must have the attributes defined since <= N.
>> ThreadInfo::from also makes sure 'stackTrace' and 'lockedMonitors'
>> attributes of version N while it can include additional attributes
>> which has been the current behavior.
>>
>> JDK-8139587 intended to support older versions of StackTraceElement
>> which does not seem a complete solution.  I reverted the fix for
>> JDK-8139587 (mostly) and removed TypeVersionMapper.  The fix constructs
>> the CompositeType for ThreadInfo and MonitorInfo of different
>> versions and used them for validation.  Minor cleanup: the static
>> final variables are renamed to all capitals.  CompatibilityTest.java
>> test is missing the copyright header.
>>
>> thanks
>> Mandy
>> [1] http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/api/java/lang/management/ThreadInfo.html#from(javax.management.openmbean.CompositeData)
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180227/232f6d57/attachment.html>