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

[Daniel Fuchs]

Hi Mandy,

Nice to see this fixed.

In ThreadInfoCompositeData::initV6CompositeType

  425                     if (name.equals(STACK_TRACE)) {
  426                         ot = new ArrayType<>(1, 
StackTraceElementCompositeData.v5CompositeType());
  427                     } else if (name.equals(LOCKED_MONITORS)) {
  428                         ot = new ArrayType<>(1, 
MonitorInfoCompositeData.v5CompositeType());

This is probably OK for StackTraceElementCompositeData, but
is confusing for MonitorInfoCompositeData. I thought
MonitorInfo was added in 6? so why would it have a
v5CompositeType?

I'd suggest renaming v5 to v6 in MonitorInfoCompositeData, and also
adding a comment when calling 
StackTraceElementCompositeData.v5CompositeType()
to say that nothing changed in StackTraceElementCompositeData
between 6 and 5.

best regards,

-- daniel



On 24/02/2018 00:50, mandy chung 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)
>