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

[Daniel Fuchs]

Hi Ujwal,

The java part looks good to me.

ThreadMXBean.java:

  775      * @implSpec if not implemented, the method will throw
  776      * an UnsupportedOperationException.

and

  862      * @implSpec if not implemented, the method will throw
  863      * an UnsupportedOperationException.

I wonder if a better wording wouldn't be:

   * @implSpec If not overridden in subclasses, the default
   *           implementation of this method
   *           throws an UnsupportedOperationException.

best regards,

-- daniel

On 11/08/2017 14:20, Ujwal Vangapally wrote:
> Gentle Reminder.
> 
> 
> On 8/9/2017 10:45 PM, Ujwal Vangapally wrote:
>>
>> 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
>>
>