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

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
kindly use the below link for accessing webrev

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

previously shared link is no longer accessible hence providing this new
link.

Thanks,

Ujwal.


On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Erik Gahlin
In reply to this post by Ujwal Vangapally
Looks good, not a reviewer.

Nit, saw that spaces before commas were missing in some method calls,
i.e. ",-1" instead of ", -1"

Erik

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

roger riggs
In reply to this post by Ujwal Vangapally
Hi Ujwal,

(Reviewer, but not specifically servicability).

Comments,

java/lang/management/ThreadMXBean.java:

809: It may be useful to state that the behavior is the same as {@link
#dumpAllThreads} except that the depth is limited.

828:  Do not duplicate specification of the meaning of maxDepth =
MAX_VALUE, either put the entire spec
in the @param or in the description.  Duplication creates an opportunity
for disagreement or a maintenance issue when updating.

833: terminate the {@code maxDepth} before "is negative".

As Erik comments, there should be a space after "," everywhere; both to
be consistent with existing style and the style guide.

sun/management/ThreadImpl.java:

484: Since the native code is going to check maxDepth, why is it checked
here also?
     -or- remove the check in native
490:  This could be simpler to not translate maxDepth to -1,
      Here and in the native code, passing MAX_INTEGER would dump the
entire stack, there is no need to special case it

services/threadService.cpp:
565:  checking count >= maxDepth is a sufficient test if to dump all
frames, passing MAX_INTEGER is used
       and it will dump zero should a negative number get this far.

Thanks, Roger

On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:

> kindly use the below link for accessing webrev
>
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/
>
> previously shared link is no longer accessible hence providing this
> new link.
>
> Thanks,
>
> Ujwal.
>
>
> On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:
>> Hi,
>>
>> kindly review the changes made.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8185003
>>
>> webrev:
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8185705
>>
>> Thanks,
>>
>> Ujwal.
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
In reply to this post by Erik Gahlin

Thanks for the review Erik,

I will investigate more considering Daniel's comment.

Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.

kindly use below webrev for referring changes as webrev.00 is not accessible.

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

-Ujwal




On 8/3/2017 8:00 PM, Erik Gahlin wrote:
Looks good, not a reviewer.

Nit, saw that spaces before commas were missing in some method calls, i.e. ",-1" instead of ", -1"

Erik

Hi,

kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8185003

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

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

Thanks,

Ujwal.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
In reply to this post by roger riggs
Thanks for the review Roger, please see my comments inline.


On 8/3/2017 8:23 PM, Roger Riggs wrote:

> Hi Ujwal,
>
> (Reviewer, but not specifically servicability).
>
> Comments,
>
> java/lang/management/ThreadMXBean.java:
>
> 809: It may be useful to state that the behavior is the same as {@link
> #dumpAllThreads} except that the depth is limited.
>
> 828:  Do not duplicate specification of the meaning of maxDepth =
> MAX_VALUE, either put the entire spec
> in the @param or in the description.  Duplication creates an
> opportunity for disagreement or a maintenance issue when updating.
>
> 833: terminate the {@code maxDepth} before "is negative".
>
> As Erik comments, there should be a space after "," everywhere; both
> to be consistent with existing style and the style guide.
will make changes to doc as suggested.
>
> sun/management/ThreadImpl.java:
>
> 484: Since the native code is going to check maxDepth, why is it
> checked here also?
>     -or- remove the check in native
As  maxDepth value should not be negative for dumpAllThreads
sun/management/ThreadImpl.java: 484: throws IllegalArgumentException.

Check on native side can be removed but having it might help in
enforcing the convention of using only -1 for dumping all the stack frames
otherwise any negative number can be passed to dump all the frames.
> 490:  This could be simpler to not translate maxDepth to -1,
>      Here and in the native code, passing MAX_INTEGER would dump the
> entire stack, there is no need to special case it
>
> services/threadService.cpp:
> 565:  checking count >= maxDepth is a sufficient test if to dump all
> frames, passing MAX_INTEGER is used
>       and it will dump zero should a negative number get this far.
>
currently getThreadInfo method accepts maxDepth argument and it uses the
translation of maxDepth to "-1"  as it's implementation detail for
printing all stack frames,
As getThreadInfo also uses services/threadService.cpp: 565:
(dump_stack_at_safepoint) expecting it to print all stack trace elements
when maxDepth is given as "-1".
I followed same convention so that it will not affect getThreadInfo
method with maxDepth argument.
> Thanks, Roger
>

Thanks,
Ujwal

> On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:
>> kindly use the below link for accessing webrev
>>
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/
>>
>> previously shared link is no longer accessible hence providing this
>> new link.
>>
>> Thanks,
>>
>> Ujwal.
>>
>>
>> On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:
>>> Hi,
>>>
>>> kindly review the changes made.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8185003
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/
>>>
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8185705
>>>
>>> Thanks,
>>>
>>> Ujwal.
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

roger riggs
Hi Ujwal,


On 8/3/2017 12:44 PM, Ujwal Vangapally wrote:
Thanks for the review Roger, please see my comments inline.


On 8/3/2017 8:23 PM, Roger Riggs wrote:
Hi Ujwal,

(Reviewer, but not specifically servicability).

Comments,

java/lang/management/ThreadMXBean.java:

809: It may be useful to state that the behavior is the same as {@link #dumpAllThreads} except that the depth is limited.

828:  Do not duplicate specification of the meaning of maxDepth = MAX_VALUE, either put the entire spec
in the @param or in the description.  Duplication creates an opportunity for disagreement or a maintenance issue when updating.

833: terminate the {@code maxDepth} before "is negative".

As Erik comments, there should be a space after "," everywhere; both to be consistent with existing style and the style guide.
will make changes to doc as suggested.

sun/management/ThreadImpl.java:

484: Since the native code is going to check maxDepth, why is it checked here also?
    -or- remove the check in native
As  maxDepth value should not be negative for dumpAllThreads sun/management/ThreadImpl.java: 484: throws IllegalArgumentException.

Check on native side can be removed but having it might help in enforcing the convention of using only -1 for dumping all the stack frames
otherwise any negative number can be passed to dump all the frames.
There is no need for a separate indication of dumping all stack frames; a count of MAX_INTEGER
will have the same effect.

Even the specification in ThreadMXBean does not need to call out the special value for maxDepth
 as MAX_INTEGER as a special value.  It just makes the spec more complicated.
490:  This could be simpler to not translate maxDepth to -1,
     Here and in the native code, passing MAX_INTEGER would dump the entire stack, there is no need to special case it

services/threadService.cpp:
565:  checking count >= maxDepth is a sufficient test if to dump all frames, passing MAX_INTEGER is used
      and it will dump zero should a negative number get this far.

currently getThreadInfo method accepts maxDepth argument and it uses the translation of maxDepth to "-1"  as it's implementation detail for printing all stack frames,
Still, with a sufficiently large maxDepth it is indistinguishable from the 'all frames' sentinel value.
The code is just more complicated than necessary as a result.

As getThreadInfo also uses services/threadService.cpp: 565: (dump_stack_at_safepoint) expecting it to print all stack trace elements when maxDepth is given as "-1".
Yes, it has the same effect as a very large maxDepth.
I followed same convention so that it will not affect getThreadInfo method with maxDepth argument.
fine, no need to change the native code, but the same result is achieved at the java level without
the adding the complexity in the implementation of ThreadImpl:489.

Roger

Thanks, Roger


Thanks,
Ujwal
On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:
kindly use the below link for accessing webrev

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

previously shared link is no longer accessible hence providing this new link.

Thanks,

Ujwal.


On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:
Hi,

kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8185003

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

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

Thanks,

Ujwal.





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung
In reply to this post by Ujwal Vangapally

On Aug 3, 2017, at 8:24 AM, Ujwal Vangapally <[hidden email]> wrote:

Thanks for the review Erik,

I will investigate more considering Daniel's comment.

Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.


ThreadMXBean is a platform MXBean and so JDK implementation is the one implementing it.  The real question here is that when MBeanServerConnection.invoke is called on this new method that does not exist in the remote MBeanServer (running on JDK 9 for example), does it get javax.management.ReflectionException?  Or something else?

Similarly, when the client gets a proxy of ThreadMXBean from a remote MBeanServer running on JDK 9 VM, and it calls this method, what exception does it get?  We may need to update the spec to indicate this error cases if it’s not clear.  The notes in ManagementFactory.newPlatformMXBeanProxy covers some cases due to the difference in the client/server are running on.

Mandy

kindly use below webrev for referring changes as webrev.00 is not accessible.

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

-Ujwal




On 8/3/2017 8:00 PM, Erik Gahlin wrote:
Looks good, not a reviewer.

Nit, saw that spaces before commas were missing in some method calls, i.e. ",-1" instead of ", -1"

Erik

Hi,

kindly review the changes made.

https://bugs.openjdk.java.net/browse/JDK-8185003

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

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

Thanks,

Ujwal.




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel Fuchs
Hi Mandy,

On 03/08/17 21:04, Mandy Chung wrote:

>> Adding a public method to an interface is an incompatible source
>> change unless there is a default body. On the other hand I am not sure
>> how MXBean proxies will work when proxying an interface containing a
>> default body. It would be interesting to check this.
>>
>
> ThreadMXBean is a platform MXBean and so JDK implementation is the one
> implementing it.  The real question here is that when
> MBeanServerConnection.invoke is called on this new method that does not
> exist in the remote MBeanServer (running on JDK 9 for example), does it
> get javax.management.ReflectionException?  Or something else?

I believe that will be a ReflectionException:
http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A-

> Similarly, when the client gets a proxy of ThreadMXBean from a remote
> MBeanServer running on JDK 9 VM, and it calls this method, what
> exception does it get?  We may need to update the spec to indicate this
> error cases if it’s not clear.  The notes in
> ManagementFactory.newPlatformMXBeanProxy covers some cases due to the
> difference in the client/server are running on.

AFAIK that should be handled by the proxy code - I'd expect that you
will get an UndeclaredThrowableException wrapping the ReflectionException.
http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308

>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung

> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]> wrote:
>
> Hi Mandy,
>
> On 03/08/17 21:04, Mandy Chung wrote:
>>> Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.
>>>
>> ThreadMXBean is a platform MXBean and so JDK implementation is the one implementing it.  The real question here is that when MBeanServerConnection.invoke is called on this new method that does not exist in the remote MBeanServer (running on JDK 9 for example), does it get javax.management.ReflectionException?  Or something else?
>
> I believe that will be a ReflectionException:
> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A-
>
>> Similarly, when the client gets a proxy of ThreadMXBean from a remote MBeanServer running on JDK 9 VM, and it calls this method, what exception does it get?  We may need to update the spec to indicate this error cases if it’s not clear.  The notes in ManagementFactory.newPlatformMXBeanProxy covers some cases due to the difference in the client/server are running on.
>
> AFAIK that should be handled by the proxy code - I'd expect that you
> will get an UndeclaredThrowableException wrapping the ReflectionException.
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308

Thanks.  We should add tests for that.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung
In reply to this post by Daniel Fuchs

> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]> wrote:
>
> Hi Mandy,
>
> On 03/08/17 21:04, Mandy Chung wrote:
>>> Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.
>>>
>> ThreadMXBean is a platform MXBean and so JDK implementation is the one implementing it.  The real question here is that when MBeanServerConnection.invoke is called on this new method that does not exist in the remote MBeanServer (running on JDK 9 for example), does it get javax.management.ReflectionException?  Or something else?
>
> I believe that will be a ReflectionException:
> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A-
>
>> Similarly, when the client gets a proxy of ThreadMXBean from a remote MBeanServer running on JDK 9 VM, and it calls this method, what exception does it get?  We may need to update the spec to indicate this error cases if it’s not clear.  The notes in ManagementFactory.newPlatformMXBeanProxy covers some cases due to the difference in the client/server are running on.
>
> AFAIK that should be handled by the proxy code - I'd expect that you
> will get an UndeclaredThrowableException wrapping the ReflectionException.
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308

Thanks. That’s what I think but need assurance. We should add tests to verify.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
Thanks for the review Daniel, Mandy, Roger, Erik.

I will make changes accordingly and come up with new webrev soon.

-Ujwal


On 8/4/2017 5:42 AM, Mandy Chung wrote:

>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]> wrote:
>>
>> Hi Mandy,
>>
>> On 03/08/17 21:04, Mandy Chung wrote:
>>>> Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.
>>>>
>>> ThreadMXBean is a platform MXBean and so JDK implementation is the one implementing it.  The real question here is that when MBeanServerConnection.invoke is called on this new method that does not exist in the remote MBeanServer (running on JDK 9 for example), does it get javax.management.ReflectionException?  Or something else?
>> I believe that will be a ReflectionException:
>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A-
>>
>>> Similarly, when the client gets a proxy of ThreadMXBean from a remote MBeanServer running on JDK 9 VM, and it calls this method, what exception does it get?  We may need to update the spec to indicate this error cases if it’s not clear.  The notes in ManagementFactory.newPlatformMXBeanProxy covers some cases due to the difference in the client/server are running on.
>> AFAIK that should be handled by the proxy code - I'd expect that you
>> will get an UndeclaredThrowableException wrapping the ReflectionException.
>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308
> Thanks. That’s what I think but need assurance. We should add tests to verify.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung
I haven't reviewed the change.  I will look at the new webrev next
week.  I think this getThreadInfo method:

    getThreadInfo(long[] ids, boolean lockedMonitors, boolean lockedSynchronizers);

should allow specifying the maxDepth too.

Mandy


On 8/3/17 11:30 PM, Ujwal Vangapally wrote:

> Thanks for the review Daniel, Mandy, Roger, Erik.
>
> I will make changes accordingly and come up with new webrev soon.
>
> -Ujwal
>
>
> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>> wrote:
>>>
>>> Hi Mandy,
>>>
>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>> Adding a public method to an interface is an incompatible source
>>>>> change unless there is a default body. On the other hand I am not
>>>>> sure how MXBean proxies will work when proxying an interface
>>>>> containing a default body. It would be interesting to check this.
>>>>>
>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>> one implementing it.  The real question here is that when
>>>> MBeanServerConnection.invoke is called on this new method that does
>>>> not exist in the remote MBeanServer (running on JDK 9 for example),
>>>> does it get javax.management.ReflectionException?  Or something else?
>>> I believe that will be a ReflectionException:
>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>
>>>
>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>> what exception does it get?  We may need to update the spec to
>>>> indicate this error cases if it’s not clear.  The notes in
>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>> the difference in the client/server are running on.
>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>> will get an UndeclaredThrowableException wrapping the
>>> ReflectionException.
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>
>> Thanks. That’s what I think but need assurance. We should add tests
>> to verify.
>>
>> Mandy
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
yes will include that in the new webrev.

Thanks,

Ujwal.


On 8/4/2017 8:08 PM, mandy chung wrote:

> I haven't reviewed the change.  I will look at the new webrev next
> week.  I think this getThreadInfo method:
>
>    getThreadInfo(long[] ids, boolean lockedMonitors, boolean
> lockedSynchronizers);
>
> should allow specifying the maxDepth too.
>
> Mandy
>
>
> On 8/3/17 11:30 PM, Ujwal Vangapally wrote:
>> Thanks for the review Daniel, Mandy, Roger, Erik.
>>
>> I will make changes accordingly and come up with new webrev soon.
>>
>> -Ujwal
>>
>>
>> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi Mandy,
>>>>
>>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>>> Adding a public method to an interface is an incompatible source
>>>>>> change unless there is a default body. On the other hand I am not
>>>>>> sure how MXBean proxies will work when proxying an interface
>>>>>> containing a default body. It would be interesting to check this.
>>>>>>
>>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>>> one implementing it.  The real question here is that when
>>>>> MBeanServerConnection.invoke is called on this new method that
>>>>> does not exist in the remote MBeanServer (running on JDK 9 for
>>>>> example), does it get javax.management.ReflectionException?  Or
>>>>> something else?
>>>> I believe that will be a ReflectionException:
>>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>>
>>>>
>>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>>> what exception does it get?  We may need to update the spec to
>>>>> indicate this error cases if it’s not clear.  The notes in
>>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>>> the difference in the client/server are running on.
>>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>>> will get an UndeclaredThrowableException wrapping the
>>>> ReflectionException.
>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>>
>>> Thanks. That’s what I think but need assurance. We should add tests
>>> to verify.
>>>
>>> Mandy
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
In reply to this post by Mandy Chung
Hi,

below is the link to new webrev incorporating review comments.

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

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 ?

Thanks,
Ujwal



On 8/4/2017 5:42 AM, Mandy Chung wrote:

>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]> wrote:
>>
>> Hi Mandy,
>>
>> On 03/08/17 21:04, Mandy Chung wrote:
>>>> Adding a public method to an interface is an incompatible source change unless there is a default body. On the other hand I am not sure how MXBean proxies will work when proxying an interface containing a default body. It would be interesting to check this.
>>>>
>>> ThreadMXBean is a platform MXBean and so JDK implementation is the one implementing it.  The real question here is that when MBeanServerConnection.invoke is called on this new method that does not exist in the remote MBeanServer (running on JDK 9 for example), does it get javax.management.ReflectionException?  Or something else?
>> I believe that will be a ReflectionException:
>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A-
>>
>>> Similarly, when the client gets a proxy of ThreadMXBean from a remote MBeanServer running on JDK 9 VM, and it calls this method, what exception does it get?  We may need to update the spec to indicate this error cases if it’s not clear.  The notes in ManagementFactory.newPlatformMXBeanProxy covers some cases due to the difference in the client/server are running on.
>> AFAIK that should be handled by the proxy code - I'd expect that you
>> will get an UndeclaredThrowableException wrapping the ReflectionException.
>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308
> Thanks. That’s what I think but need assurance. We should add tests to verify.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel Fuchs
Hi Ujwal,

I have made a small experiments and it seems that having
a default body doesn't prevent an interface method from
being called from either a JMX or a Platform proxy.

So in the light of this I'll suggest to add a default body
that throw UnsupportedOperationException to the new methods
added to the ThreadMXBean interface, and add an @implSpec
to specify that if not implemented, the method will throw
an UnsupportedOperationException.

This should guarantee better backward compatibility in case
someone has implemented the ThreadMXBean interface.

Mandy, would you agree?

best regards

-- daniel

On 08/08/2017 09:27, 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/
>
> 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 ?
>
> Thanks,
> Ujwal
>
>
>
> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>> wrote:
>>>
>>> Hi Mandy,
>>>
>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>> Adding a public method to an interface is an incompatible source
>>>>> change unless there is a default body. On the other hand I am not
>>>>> sure how MXBean proxies will work when proxying an interface
>>>>> containing a default body. It would be interesting to check this.
>>>>>
>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>> one implementing it.  The real question here is that when
>>>> MBeanServerConnection.invoke is called on this new method that does
>>>> not exist in the remote MBeanServer (running on JDK 9 for example),
>>>> does it get javax.management.ReflectionException?  Or something else?
>>> I believe that will be a ReflectionException:
>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>
>>>
>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>> what exception does it get?  We may need to update the spec to
>>>> indicate this error cases if it’s not clear.  The notes in
>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>> the difference in the client/server are running on.
>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>> will get an UndeclaredThrowableException wrapping the
>>> ReflectionException.
>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>
>> Thanks. That’s what I think but need assurance. We should add tests to
>> verify.
>>
>> Mandy
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ujwal Vangapally
Thanks for the review Daniel.

I also verified Daniel's suggestion, it works fine and it helps in
better backward compatibility.

if it is ok to have a default body in ThreadMXBean interface I will send
a new webrev incorporating that change.

-Ujwal


On 8/8/2017 2:52 PM, Daniel Fuchs wrote:

> Hi Ujwal,
>
> I have made a small experiments and it seems that having
> a default body doesn't prevent an interface method from
> being called from either a JMX or a Platform proxy.
>
> So in the light of this I'll suggest to add a default body
> that throw UnsupportedOperationException to the new methods
> added to the ThreadMXBean interface, and add an @implSpec
> to specify that if not implemented, the method will throw
> an UnsupportedOperationException.
>
> This should guarantee better backward compatibility in case
> someone has implemented the ThreadMXBean interface.
>
> Mandy, would you agree?
>
> best regards
>
> -- daniel
>
> On 08/08/2017 09:27, 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/
>>
>> 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 ?
>>
>> Thanks,
>> Ujwal
>>
>>
>>
>> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi Mandy,
>>>>
>>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>>> Adding a public method to an interface is an incompatible source
>>>>>> change unless there is a default body. On the other hand I am not
>>>>>> sure how MXBean proxies will work when proxying an interface
>>>>>> containing a default body. It would be interesting to check this.
>>>>>>
>>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>>> one implementing it.  The real question here is that when
>>>>> MBeanServerConnection.invoke is called on this new method that
>>>>> does not exist in the remote MBeanServer (running on JDK 9 for
>>>>> example), does it get javax.management.ReflectionException?  Or
>>>>> something else?
>>>> I believe that will be a ReflectionException:
>>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>>
>>>>
>>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>>> what exception does it get?  We may need to update the spec to
>>>>> indicate this error cases if it’s not clear.  The notes in
>>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>>> the difference in the client/server are running on.
>>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>>> will get an UndeclaredThrowableException wrapping the
>>>> ReflectionException.
>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>>
>>> Thanks. That’s what I think but need assurance. We should add tests
>>> to verify.
>>>
>>> Mandy
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung
In reply to this post by Daniel Fuchs
Throwing UOE would be clearer than throwing UndeclaredThrowableException
and ReflectionException.

However MBean proxy simply forwards the call to the remote MBeanServer
and calls invoke or getAttribute/setAttribute. NoSuchMethodException
will be thrown in the remote VM and the client would get  
UndeclaredThrowableException or ReflectionException.  I don't see how
the client will invoke the default method unless the JMX
MBeanInvocationHandler catches the exception and have special handling
to the default methods.

For the case of missing method in the remote VM but the client has the
default method, I got UndeclaredThrowableException.  Is that what you see?

Mandy


On 8/8/17 2:22 AM, Daniel Fuchs wrote:

> Hi Ujwal,
>
> I have made a small experiments and it seems that having
> a default body doesn't prevent an interface method from
> being called from either a JMX or a Platform proxy.
>
> So in the light of this I'll suggest to add a default body
> that throw UnsupportedOperationException to the new methods
> added to the ThreadMXBean interface, and add an @implSpec
> to specify that if not implemented, the method will throw
> an UnsupportedOperationException.
>
> This should guarantee better backward compatibility in case
> someone has implemented the ThreadMXBean interface.
>
> Mandy, would you agree?
>
> best regards
>
> -- daniel
>
> On 08/08/2017 09:27, 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/
>>
>> 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 ?
>>
>> Thanks,
>> Ujwal
>>
>>
>>
>> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi Mandy,
>>>>
>>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>>> Adding a public method to an interface is an incompatible source
>>>>>> change unless there is a default body. On the other hand I am not
>>>>>> sure how MXBean proxies will work when proxying an interface
>>>>>> containing a default body. It would be interesting to check this.
>>>>>>
>>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>>> one implementing it.  The real question here is that when
>>>>> MBeanServerConnection.invoke is called on this new method that
>>>>> does not exist in the remote MBeanServer (running on JDK 9 for
>>>>> example), does it get javax.management.ReflectionException?  Or
>>>>> something else?
>>>> I believe that will be a ReflectionException:
>>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>>
>>>>
>>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>>> what exception does it get?  We may need to update the spec to
>>>>> indicate this error cases if it’s not clear.  The notes in
>>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>>> the difference in the client/server are running on.
>>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>>> will get an UndeclaredThrowableException wrapping the
>>>> ReflectionException.
>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>>
>>> Thanks. That’s what I think but need assurance. We should add tests
>>> to verify.
>>>
>>> Mandy
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel Fuchs
Hi Mandy,

On 08/08/2017 18:03, mandy chung wrote:
> Throwing UOE would be clearer than throwing UndeclaredThrowableException
> and ReflectionException.

That's what will happen if someone had a custom implementation
of ThreadMXBean which is compiled against JDK 9 but runs in JDK 10,
and therefore does not have the new methods. Then it inherits
the default body that throes UOE.

With the default body then the implementation can still be compiled
against jdk 10, without having to add the new method.

> However MBean proxy simply forwards the call to the remote MBeanServer
> and calls invoke or getAttribute/setAttribute. NoSuchMethodException
> will be thrown in the remote VM and the client would get
> UndeclaredThrowableException or ReflectionException.

You will get UndeclaredThrowableException if the remote VM is a JDK 9
VM and the client is JDK 10 VM and that's OK.

> I don't see how
> the client will invoke the default method unless the JMX
> MBeanInvocationHandler catches the exception and have special handling
> to the default methods.

That's not what we're trying to address here.

> For the case of missing method in the remote VM but the client has the
> default method, I got UndeclaredThrowableException.  Is that what you see?

That's what I would expect. I did some testing to verify that if an
interface I had a default method m and an MBean M implemented I,
providing an implementation of m, then the result you get by calling
JMX.newMXBeanProxy(connection, name, I.class).m() is actually what
is returned by M.m(), not what is returned by I.m().

This means that you can evolve MBean interfaces by adding methods
to them as long as you provide a default body (which is rather cool :-))

best regards,

-- daniel

>
> Mandy
>
>
> On 8/8/17 2:22 AM, Daniel Fuchs wrote:
>> Hi Ujwal,
>>
>> I have made a small experiments and it seems that having
>> a default body doesn't prevent an interface method from
>> being called from either a JMX or a Platform proxy.
>>
>> So in the light of this I'll suggest to add a default body
>> that throw UnsupportedOperationException to the new methods
>> added to the ThreadMXBean interface, and add an @implSpec
>> to specify that if not implemented, the method will throw
>> an UnsupportedOperationException.
>>
>> This should guarantee better backward compatibility in case
>> someone has implemented the ThreadMXBean interface.
>>
>> Mandy, would you agree?
>>
>> best regards
>>
>> -- daniel
>>
>> On 08/08/2017 09:27, 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/
>>>
>>> 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 ?
>>>
>>> Thanks,
>>> Ujwal
>>>
>>>
>>>
>>> On 8/4/2017 5:42 AM, Mandy Chung wrote:
>>>>> On Aug 3, 2017, at 2:10 PM, Daniel Fuchs <[hidden email]>
>>>>> wrote:
>>>>>
>>>>> Hi Mandy,
>>>>>
>>>>> On 03/08/17 21:04, Mandy Chung wrote:
>>>>>>> Adding a public method to an interface is an incompatible source
>>>>>>> change unless there is a default body. On the other hand I am not
>>>>>>> sure how MXBean proxies will work when proxying an interface
>>>>>>> containing a default body. It would be interesting to check this.
>>>>>>>
>>>>>> ThreadMXBean is a platform MXBean and so JDK implementation is the
>>>>>> one implementing it.  The real question here is that when
>>>>>> MBeanServerConnection.invoke is called on this new method that
>>>>>> does not exist in the remote MBeanServer (running on JDK 9 for
>>>>>> example), does it get javax.management.ReflectionException?  Or
>>>>>> something else?
>>>>> I believe that will be a ReflectionException:
>>>>> http://download.java.net/java/jdk9/docs/api/javax/management/MBeanServerConnection.html#invoke-javax.management.ObjectName-java.lang.String-java.lang.Object:A-java.lang.String:A- 
>>>>>
>>>>>
>>>>>> Similarly, when the client gets a proxy of ThreadMXBean from a
>>>>>> remote MBeanServer running on JDK 9 VM, and it calls this method,
>>>>>> what exception does it get?  We may need to update the spec to
>>>>>> indicate this error cases if it’s not clear.  The notes in
>>>>>> ManagementFactory.newPlatformMXBeanProxy covers some cases due to
>>>>>> the difference in the client/server are running on.
>>>>> AFAIK that should be handled by the proxy code - I'd expect that you
>>>>> will get an UndeclaredThrowableException wrapping the
>>>>> ReflectionException.
>>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/src/java.management/share/classes/javax/management/MBeanServerInvocationHandler.java#l308 
>>>>>
>>>> Thanks. That’s what I think but need assurance. We should add tests
>>>> to verify.
>>>>
>>>> Mandy
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mandy Chung


On 8/8/17 10:18 AM, Daniel Fuchs wrote:
>
> That's what will happen if someone had a custom implementation
> of ThreadMXBean which is compiled against JDK 9 but runs in JDK 10,
> and therefore does not have the new methods. Then it inherits
> the default body that throes UOE.

I'm not aware of any custom implementation of the platform mxbean. I
would expect only JVM implementations would do that.  The API was
evolved in the past and haven't heard such issue.
>
> With the default body then the implementation can still be compiled
> against jdk 10, without having to add the new method.
> :
>
> This means that you can evolve MBean interfaces by adding methods
> to them as long as you provide a default body (which is rather cool :-))
>
I understand the nice benefit.   IMO I don't think it's highly necessary
since these MBeans are expected to be provided by the platform.

Mandy
12
Loading...