RFR: JDK-8185003 JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

[Ujwal Vangapally]

Thanks for the review Mandy,

kindly see my comments inline.

webrev: 
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.03/

csr: https://bugs.openjdk.java.net/browse/JDK-8185705

Ujwal


On 8/9/2017 5:23 AM, mandy chung wrote:
>
>
>
> On 8/8/17 1:27 AM, Ujwal Vangapally wrote:
>> Hi,
>>
>> below is the link to new webrev incorporating review comments.
>>
>> webrev: 
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.02/
>>
> 769 * The Behaviour is same as {@link #getThreadInfo(long[], boolean, 
> boolean)}
> 770 * except that the stack trace depth is limited.
> 771 *
> Alternatively you can copy the javadoc from getThreadInfo(long[], 
> boolean, boolean) to the new method, saying:
>
> Returns the thread info for each thread whose ID is in the input array 
> {@code ids}, with stack trace of the specified maxinum number of 
> elements and synchronization information.
>
> getThreadInfo(long[], boolean, boolean) can replace most of the 
> javadoc by saying:  This is equivalent to calling:
> getThreadInfo(ids, lockedMonitors, lockedSynchronizers, Integer.MAX_VALUE)
>
> Same applies to dumpAllThreads.
>
did it as you suggested but it is not shown very clearly in webrev, 
kindly take a look.
> I did look at grepcode and see any custom implementation of 
> ThreadMXBean.  There are cases that extends ThreadMXBean which mostly 
> attempts to call com.sun.management.ThreadMXBean without referencing 
> the type.  They are not custom implementation of ThreadMXBean.
>
> I agree with Daniel's suggestion to make the new methods with a 
> default implementation throwing UOE.  This might make existing 
> implementation easier to migrate.
>
made them default methods.
> sun/management/ThreadImpl.java
> 495 if(maxDepth < 0) { 500 return dumpThreads0(null, lockedMonitors, 
> lockedSynchronizers,maxDepth);
> missing space between if and ( and ,
>
changed it.
> src/java.management/share/native/libmanagement/ThreadImpl.c
>    long line 138.  You can wrap it.
>
did it.
> src/share/vm/services/jmm.h
>    This changes the interface and so you should add JMM_VERSION_10 
> even we don't support mixing hotspot with of different version of JDK 
> libraries.
>
not sure if I made this change correctly please confirm by seeing webrev.
> src/share/vm/services/management.cpp
>    line 1163 is quite long line.  Can you wrap it?
>
did it.
>    jmm_DumpThreads - it should probably validate maxDepth as 
> jmm_GetThreadInfo does.
we can do it as an additional check, I didn't find anything which calls 
jmm_DumpThreads with negative value for maxDepth.
As we are verifying maxDepth value at java level and I didn't use -1 as 
a special case for maxDepth to print full stack.
But I see no harm in adding an additional check at native level.

>> verified that
>>
>> MBeanServerConnection.invoke throws ReflectionException when invoked 
>> with a method that doesn't exist in remote MBean server.
>>
>> UndeclaredThrowableException will be throwed when a client gets proxy 
>> of remote MBean server and calls a method that doesn't exist on 
>> remote Mbean server.
>>
>> do we need to develop Automated tests for verifying above cases ?
>>
> It'd be useful unless there is existing JMX tests covering this 
> scenario.  This is more on JMX framework and  it's okay to separate a 
> new issue for adding these new tests.
>
> We should re-read the specification and see if spec clarification is 
> needed.
will do this soon.
>
> Mandy

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