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

Tue Jun 18 03:01:55 PDT 2013

On 06/07/2013 11:07 AM, Daniel Fuchs wrote:
>> 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).

Yes, I've run all the regtests and JCKs for api/javax_management and
api/java_lang. No regressions were detected.

> 
> 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?

I've added the tests for the proper behaviour when the
"com.sun.jmx.mbeans.allowNonPublic" system property is set to true.

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

-JB-

> 
> 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-
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
> 