jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

[Daniel Fuchs]

> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04

Hi Jaroslav,

This looks good to me.

I assume you've been running both the java.lang & javax.management
unit tests & JCK (java.lang JCK also has some test cases that
indirectly involve JMX introspection).

Also, maybe you could add a test to check that
the system property which allows to revert to the old
behavior is taken into account?

good work!

-- daniel


On 6/7/13 9:13 AM, Jaroslav Bachorik wrote:
> On Thu 06 Jun 2013 06:06:52 PM CEST, shanliang wrote:
>> Jaroslav Bachorik wrote:
>>> On Thu 06 Jun 2013 05:22:31 PM CEST, shanliang wrote:
>>>
>>>> Jaroslav,
>>>>
>>>> It is now OK for me about the MBean interface searching in the
>>>> Introspector.
>>>>
>>>> Here is my comment on JMX.java:
>>>> 206 -- 212 you added a call
>>>>      Introspector.testComplianceMBeanInterface(interfaceClass);
>>>>
>>>> It is better to move this call to:
>>>>      MBeanServerInvocationHandler.newProxyInstance
>>>> because the real job is done in newProxyInstance and it could be
>>>> directly called by anyone.
>>>>
>>>
>>> Hm, wouldn't it be better to move the actual logic from
>>> MBeanServerInvocationHandler.newProxyInstance to JMX.newMBeanProxy and
>>> delegate the MBSIH.newProxyInstance back to JMX.newMBeanProxy ? This
>>> way it would be consistent with the way JMX.newMXBeanProxy is written.
>>>
>> I have no opinion here, this is an implementation detail, anyway we
>> have to keep both JMX.newMBeanProxy and
>> MBeanServerInvocationHandler.newProxyInstance
>
> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04
>
> I've moved the newMBeanProxy() logic to JMX to keep it consistent  with
> JMX.newMXBeanProxy(); MBSIH.newProxyInstance() is just forwarded to
> JMX.newMBeanProxy()
>
> -JB-
>
>>
>> Shanliang
>>>
>>>> All others are OK for me.
>>>>
>>>
>>> Thanks.
>>>
>>> -JB-
>>>
>>>
>>>> Shanliang
>>>>
>>>> Jaroslav Bachorik wrote:
>>>>
>>>>> On Wed 05 Jun 2013 07:54:10 PM CEST, shanliang wrote:
>>>>>
>>>>>
>>>>>> Daniel Fuchs wrote:
>>>>>>
>>>>>>
>>>>>>> On 6/5/13 3:55 PM, Jaroslav Bachorik wrote:
>>>>>>>
>>>>>>>
>>>>>>>>> class A extends B { ...}
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> class B implements AMBean {...}
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> Yes, I see it now. However, when you check the JMX specification, page
>>>>>>>> 50 onwards, the current implementation does not seem to be correct.
>>>>>>>>
>>>>>>>> "3. If MyClass is an instance of the DynamicMBean interface, then
>>>>>>>> MyClassMBean is
>>>>>>>> ignored. If MyClassMBean is not a public interface, it is not a JMX
>>>>>>>> manageable
>>>>>>>> resource. If the MBean is an instance of neither MyClassMBean nor
>>>>>>>> DynamicMBean, the inheritance tree of MyClass is examined, looking
>>>>>>>> for the
>>>>>>>> nearest superclass that implements its own MBean interface.
>>>>>>>> a. If there is an ancestor called SuperClass that is an instance of
>>>>>>>> SuperClassMBean, the design patterns are used to derive the
>>>>>>>> attributes and
>>>>>>>> operations from SuperClassMBean. In this case, the MBean MyClass then
>>>>>>>> has the same management interface as the MBean SuperClass. If
>>>>>>>> SuperClassMBean is not a public interface, it is not a JMX manageable
>>>>>>>> resource.
>>>>>>>> b. When there is no superclass with its own MBean interface, MyClass is
>>>>>>>> not a
>>>>>>>> Standard MBean."
>>>>>>>>
>>>>>>>> According to the specification the correct MBean interface for
>>>>>>>>
>>>>>>>>     class A extends B { ...}
>>>>>>>>     class B implements BMBean, AMBean
>>>>>>>>
>>>>>>>> would be BMBean
>>>>>>>>
>>>>>>>>
>>>>>>> Hi Jaroslav,
>>>>>>>
>>>>>>> Given that A is an instance of AMBean I think that according to the
>>>>>>> specification the correct interface should be AMBean.
>>>>>>> It's true that the JMX Specification does not explicitly speak of this
>>>>>>> case - but neither does it forbid it.
>>>>>>>
>>>>>>> My advice would therefore be to clarify the spec on this point,
>>>>>>> if that's needed - rather than risking the introduction of
>>>>>>> incompatibilities.
>>>>>>>
>>>>>>> -- daniel
>>>>>>>
>>>>>>>
>>>>>> Look at the spec 1.4:
>>>>>> ------
>>>>>> 2. If the MyClass  MBean is an instance of a MyClassMBean interface,
>>>>>> then only the methods listed in, or inherited by, the interface are
>>>>>> considered among all the methods of, or inherited by, the MBean. The
>>>>>> design patterns are then used to identify the attributes and
>>>>>> operations from the method names in the MyClassMBean interface and its
>>>>>> ancestors. In other words, MyClass is a standard MBean
>>>>>> ------
>>>>>>
>>>>>> Here A is an instance of AMBean, according to 2), A is a standard
>>>>>> MBean and  AMBean must be taken,
>>>>>>
>>>>>> 3) specifies the condition as "If the MBean is an instance of neither
>>>>>> MyClassMBean nor DynamicMBean", our example is out of this condition,
>>>>>> so should not apply 3) to our example.
>>>>>>
>>>>>>
>>>>> Ok. I've reverted to the original implementation.
>>>>>
>>>>> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.03/
>>>>>
>>>>> The whole MBean interface inferring algorithm is a bit of black magic,
>>>>> though. I mean, checking all the interfaces implemented by all the
>>>>> classes in the inheritance hierarchy is counter-intuitive. I mean, why
>>>>> would you do something like :
>>>>>    MyServiceIfc extends ObscureIfc extends ServiceMBean
>>>>>    Service implements MyServiceIfc
>>>>>
>>>>> and silently supposing that somewhere in the interface inheritance
>>>>> hierarchy just happen to be a properly named interface and my Service
>>>>> would become a managed resource. Not mentioning the fact, that the
>>>>> current implementation will fail to resolve the ServiceMBean as the
>>>>> MBean interface - it stops checking by ObscureIfc.
>>>>>
>>>>> It would be much easier for the user if he just specified the MBean
>>>>> interface alongside the implementation class (... implements ...,
>>>>> ServiceMBean) cleanly indicating that an object is a managed resource.
>>>>>
>>>>> But, what you gonna do ... changing the spec would probably open  a
>>>>> whole another can of worms and since nobody is complaining about the
>>>>> current implementation we can just leave it as it is.
>>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>>
>>>>>> Shanliang
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> and for
>>>>>>>>     class A extends B { ...}
>>>>>>>>     class B implements AMBean {...}
>>>>>>>>
>>>>>>>> is not defined; neither B or A are manageable resources.
>>>>>>>>
>>>>>>>> As I said, the jtreg and jck test does not seem to mind which
>>>>>>>> implementation is used, they all pass happily. I would prefer bringing
>>>>>>>> the implementation in sync with the specification.
>>>>>>>>
>>>>>>>> -JB-
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>
>
>