jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Jun 18 03:25:35 PDT 2013
Hi Jaroslav,
> 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/
Thanks for adding the test. I haven't looked at the source again,
I trust nothing changed there. Some issues in the tests:
MBeanFallbackTest.java:
57 if (failures > 0) {
58 System.exit(1);
59 }
I think
throw new Exception("TEST FAILURES: " + failures);
would be more appropriate.
Also I see that you have an @run main line - I believe it
should be @run main/othervm since the test sets some
system properties.
MXBeanFallbackTest.java:
30 * @author Eamonn McManus
Is that a copy paste issue?
47 System.in.read();
Left over from debugging session - right ;-) ?
I see that you have an @run main line there too - I
believe it should be @run main/othervm since this
test also sets some system properties.
JMXProxyFallbackTest.java:
63 if (failures > 0) {
64 System.exit(1);
65 }
should throw new Exception("TEST FAILURES: " + failures);
instead.
I see that you have an @run main line there too - I
believe it should be @run main/othervm since this
test also sets some system properties.
JMXProxyTest.java:
120 if (failures > 0) {
121 System.exit(1);
122 }
should throw new Exception("TEST FAILURES: " + failures);
instead.
best regards,
-- daniel
On 6/18/13 12:01 PM, Jaroslav Bachorik wrote:
> 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-
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
More information about the serviceability-dev
mailing list