Jaroslav Bachorik wrote:
On Wed 05 Jun 2013 11:34:05 AM CEST, shanliang wrote:
Jaroslav Bachorik wrote:
On 05/30/2013 09:32 AM, Jaroslav Bachorik wrote:
On Wed 29 May 2013 07:44:34 PM CEST, Daniel Fuchs wrote:
On 5/29/13 7:17 PM, Jaroslav Bachorik wrote:
On Wed 29 May 2013 05:33:21 PM CEST, Eamonn McManus wrote:
I would recommend against changing the code to do additional calls to
Class.forName during MBean introspection. As I recall we made the
opposite change some years ago, both because Class.forName can be slow
(it may call out to a user ClassLoader) and because it is a potential
source of security problems.
Thanks. I was trying to dig some history from mercurial but couldn't.
Walking through all the related interfaces is equally acceptable - I've
tried both of the solutions and they test well with the regtests.
I am still puzzled by the current implementation which will fail to
locate the correct MBean interface in eg.
<<CInterface>> extends <<BInterface>> extends <<ServiceMBean>>
ClassA extends Service implements <<CInterface>>
as the process would stop on <<BInterface>> (checks the superclass of
the ClassA, checks all the interfaces implemented by the Service class,
checks all the interfaces extended by <<CInterface>>) which plainly
does not conform to the MBean interface naming convention and would
miss the <<ServiceMBean>> interface.
Hi Jaroslav,
<<Service>> would have to implement <<ServiceMBean>> either
directly or indirectly.
So the current implementation is correct.
If <<ServiceMBean>> is not assignable from <<Service>> then
<<ServiceMBean>> is not an MBean interface for ClassA.
Actually, when you do
ClassA extends Service implements <<BInterface>>
the Introspector will return <<ServiceMBean>> as the standard mbean
interface for ClassA. I've just tried it on a simple project to make
sure I understand the code correctly. The puzzle is which behaviour is
correct? Either all the levels of the interface hierarchy should be
checked for the [className]MBean interfaces or none, I guess. However,
I can not find anything in the spec related to this case.
I've updated the webrev not to use Class.forName() when inferring the
MBean interface to use.
However, I've changed the way the interface hierarchy is treated -
originally, all the interfaces of a certain class and their direct super
interfaces were checked for the MBean interface candidates (with
[class.Name]MBean class name) - but only one level of the hierarchy was
checked. The new implementation will check only the direct interfaces.
I am still not sure whether the interface hierarchy should be searched
for the appropriately named interfaces or not. The spec does not say
anything about this and all the jtreg and jck tests are indifferent to
my changes. But I am sure that if it is supposed to search the interface
hierarchy it should not stop after the first level has been checked, as
is done by the current implementation.
Your modification changes the current JMX behavior, if the spec does
not say about interface searching, I think it is better to keep the
current behavior for the implementation compatibility.
Here are examples:
1) A extends B implements AMBean, BMBean
the current JMX will get AMBean for the class A, but BMBean will be
got with your modification.
Sorry I was not clear here, I meant:
class A extends B { ...}
class B implements BMBean, AMBean
No, it will return AMBean. Firstly, the A is interrogated and if there
is an AMBean interface in the list of implemented interfaces it is
returned.
2) A extends B implements AMBean
the current JMX will get AMBean for the class A, but which mbean
interface will be found with your modification? I think it will be null.
No, it will return AMBean.
class A extends B { ...}
class B implements AMBean {...}
The only thing I've changed is that if you have BMBean extends AMBean
and A implements BMBean the AMBean won't be returned. While the current
implementation would return AMBean if we add CMBean extends BMBean and
then A implements CMBean it will fail to return the AMBean even though
it is virtually the same situation. IMO, the implementation should be
consistent whether it walks up the interface hierarchy to find the
MBean interface or not - and not to choose an arbitrary number of
levels that will be checked (currently 2).
367 if ((Modifier.isPublic(mbeanInterface.getModifiers()) ||
368 MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN) &&
369 clMBeanName.equals(mbeanInterface.getName())) {
370 return mbeanInterface;
371 }
As I indicated in my previous mail, better to write as:
if (clMBeanName.equals(mbeanInterface.getName() &&
(Modifier.isPublic(mbeanInterface.getModifiers()) ||
MBeanAnalyzer.ALLOW_NONPUBLIC_MBEAN)) {
checking name first because it must be cheaper here.
I am not sure - clMBeanName.equals(mbeanInterface.getName()) involves
character by character comparison. MBeanAnalyzer.ALLOW_NON_PUBLIC_MBEAN
is simple static field access and if mbeanInterface.getModifiers()
isn't horribly slow we should be better off with my version. I can
profile the code to find out the difference.
I meant that Call name.equals would filter off all interfaces except
those which had same name (at most only one in our case?), then we
needed only to call Modifier.isPublic(mbeanInterface.getModifiers() for
those same named interfaces.
Modifier.isPublic(mbeanInterface.getModifiers() would returns true for
all public interfaces so we had to call name.equals for all public
interfaces until got same named interface.
Shanliang
-JB-
Shanliang
Webrev: http://cr.openjdk.java.net/~jbachorik/8010285/webrev.02
-JB-
-JB-
You can work around that by wrapping an instance of ClassA
in an instance of javax.management.StandardMBean, and by
specifying <<ServiceMBean>>.class as the MBean interface
in the constructor.
Hope this helps,
-- daniel
-JB-
Éamonn
2013/5/29 Jaroslav Bachorik <jaroslav.bacho...@oracle.com
<mailto:jaroslav.bacho...@oracle.com>>
Updated webrev -
http://cr.openjdk.java.net/~jbachorik/8010285/webrev.01
It adds regtests and takes care of the comments from David and
Shanliang.
-JB-
On 05/28/2013 04:22 PM, Jaroslav Bachorik wrote:
> The fix enforces the management interfaces (read MBean and
MXBean
> interfaces) being public. While this is defined in the
specification it
> was not enforced in any way and it was allowed to create MBeans
for eg.
> private MBean interfaces.
>
> The fix adds checks when creating and registering MBeans and
throws
> javax.management.NotCompliantMBeanException when a user tries to
create
> an MBean with non-public management interface.
>
> Since this change can cause problems for users having non-public
> management interfaces a system property is introduced that will
revert
> to the old behaviour when set
(com.sun.jmx.mbeans.allowNonPublic).
>
> Thanks,
>
> -JB-
>
