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

[mandy chung]

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.


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

> 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 
> <mailto:mandy.chung at oracle.com>> wrote:
>
>     Webrev at:
>        http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.01
>     <http://cr.openjdk.java.net/%7Emchung/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)
>     <http://cr.openjdk.java.net/%7Emchung/jdk11/webrevs/8198253/api/java/lang/management/ThreadInfo.html#from%28javax.management.openmbean.CompositeData%29>
>
>

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