Bug 49045 - JMX Enhancement- Provision of MBeanFactory createStandardService
JMX Enhancement- Provision of MBeanFactory createStandardService
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
PC All
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
: 49062 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2010-04-04 05:40 UTC by chamith buddhika
Modified: 2010-04-14 08:07 UTC (History)
0 users



Attachments
Adding createStandardService method to MBeanFactory (8.85 KB, patch)
2010-04-04 06:24 UTC, chamith buddhika
Details | Diff
Adding createStandardService method to MBeanFactory (13.67 KB, patch)
2010-04-13 07:32 UTC, chamith buddhika
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chamith buddhika 2010-04-04 05:40:57 UTC
Currently MBeanFactory doesn't have a method for creating a new StandardService MBean. This would become an issue for future enhancements of JMX support in Tomcat. As per discussions with Mark, this enhancement is suggested as a starting point for possible future enhancements in JMX support.
Comment 1 chamith buddhika 2010-04-04 06:24:32 UTC
Created attachment 25228 [details]
Adding createStandardService method to MBeanFactory

This initial version of the patch adds createStandarService method. However I am having some issues with the usage of the MBean with respect to MBeanFactory. In the case of MBeanFactory both the managed resource and the associated MBean are MBeanFactory instances. But only resource MBeanFactory object is properly populated with a reference to the Server in its container field. The associated MBean instance which gets registered in the MBean Server (created by JMX infrastructure) doesn't get populated with this field since it is being created using reflection in createMBean() method in ManagedBean class. 

Now when the MBean createStandardService method is invoked using JMX (which depends on container field being properly set - in this case the reference to the single Server instance) the MBean and not the managed resource is invoked causing a null pointer exception. The below code snippet in BaseModelBean causes the above behaviour.

// Invoke the selected method on the appropriate object
Object result = null;
try {
    if( method.getDeclaringClass().isAssignableFrom( this.getClass()) ) {
       result = method.invoke(this, params );
    } else {
       result = method.invoke(resource, params);
    }
}

Here as can be seen the precedence is given to the MBean object instead of the associated resource object. Can somebody comment on the reason for the above logic? 

To get the resource MBeanFactory instance invoked I have included a check as follows in the above condition which I think is not the way to go.
 
 if( method.getDeclaringClass().isAssignableFrom( this.getClass())
         && !method.getDeclaringClass().equals(this.getClass()) ) {

Any ideas on this issue?
Comment 2 Mark Thomas 2010-04-06 21:32:20 UTC
A couple of comments.

You should be targeting your patches at Tomcat 7 aka trunk. Please raise bugs against Tomcat 7 too.

The Factory can be associated with a Server or a Service. If it is associated with a Service it isn't valid to try adding another Service. That should probably throw an Exception.

If the Factory is associated with a Server then casting container to Server should work.

Why are you creating an Engine too? Only a Service should be created. Creating an Engine should be a separate method.

I haven't looked at this in much detail but I'd be surprised if modifying the modeler code is necessary. I'd find a method on a bean that does work (eg the addAlias() method on a host) and see if anything is different about the way it is set up compared to the MBeanFactory.
Comment 3 chamith buddhika 2010-04-07 11:45:01 UTC
(In reply to comment #2)
Hi Mark,

> You should be targeting your patches at Tomcat 7 aka trunk. Please raise bugs
> against Tomcat 7 too.

I will raise a bug for Tomcat 7 as well.

> The Factory can be associated with a Server or a Service. If it is associated
> with a Service it isn't valid to try adding another Service. That should
> probably throw an Exception.

Here what I was thinking was since technically a Server can contain more than one service that it would be possible to add another service to the server associated with the current service component in the MBeanFactory. I will change it to throw an exception.

> Why are you creating an Engine too? Only a Service should be created. Creating
> an Engine should be a separate method.

Please correct me if I am wrong here. Since one service is always mapped only to one Engine component it would be not possible to use an existing registered Engine component with the newly created service without any side effects. Hence the creation of a brand new engine within the new service. I saw this has been done in the removed codes using ServerFactory as well. Another option would be that user can seperately create an Engine component prior to Service creation and register it and then use it to create the new Service. I will add a separate method to create an Engine component in that case.

> I haven't looked at this in much detail but I'd be surprised if modifying the
> modeler code is necessary. I'd find a method on a bean that does work (eg the
> addAlias() method on a host) and see if anything is different about the way it
> is set up compared to the MBeanFactory.

I will look in to this further. 

Thanks
Comment 4 Mark Thomas 2010-04-07 19:20:00 UTC
(In reply to comment #3)
> I will raise a bug for Tomcat 7 as well.
That wasn't quite what I meant. I meant that this bug should be against Tomcat 7, not Tomcat 6. I'll move it.

> Here what I was thinking was since technically a Server can contain more than
> one service that it would be possible to add another service to the server
> associated with the current service component in the MBeanFactory. I will
> change it to throw an exception.
There are cases where there is a service with no server. Hence the exception is the better option.

> Please correct me if I am wrong here. Since one service is always mapped only
> to one Engine component it would be not possible to use an existing registered
> Engine component with the newly created service without any side effects. Hence
> the creation of a brand new engine within the new service. I saw this has been
> done in the removed codes using ServerFactory as well. Another option would be
> that user can seperately create an Engine component prior to Service creation
> and register it and then use it to create the new Service. I will add a
> separate method to create an Engine component in that case.
You are correct that there is a 1 to 1 mapping between service and engine. For simplicity, I'd still keep them separate.
Comment 5 Mark Thomas 2010-04-07 19:20:19 UTC
*** Bug 49062 has been marked as a duplicate of this bug. ***
Comment 6 Mark Thomas 2010-04-12 17:58:00 UTC
Ping. When do you expect to have an updated patch?
Comment 7 chamith buddhika 2010-04-13 07:32:51 UTC
Created attachment 25277 [details]
Adding createStandardService method to MBeanFactory

Sorry for the delay. I was busy summing up some other work. I have been looking more in to the issue of MBeanFactory.The issue was caused by the fact that MBeanFactory itself being a BaseModelBean unlike other normal classes such as StandardService etc. Although it extends BaseModelBean it doesn't utilise any of its inherited methods and it being a BaseModelBean is not leveraged in any other point in the sources. So I suggest removing 'is a' relationship between MBeanFactory and BaseModelBean and make MBeanFactory a normal class which can be managed using a BaseModelBean object. It solves the issue that both managed resource and the mbean being instances of the same class, a situation which is unique to MBeanFactory.

In this patch I have reversed the changes made to the modeler code in order to overcome the earlier issue. And additionally separate create method has been added to create an Engine component.
Comment 8 Mark Thomas 2010-04-14 08:07:06 UTC
Thanks for the analysis and updated patch.

As I reviewed the patch I realised that a lot of the complexity was caused by trying to add Service and Engine separately. When considered along side the long term aim to merge Service and Engine and the short term goal to extend Lifecycle to cover MBean registration I now think you original idea to add the two togetehr is the right way to go.

I have applied a patch based you your patches that does this and also cleans up some old methods from the descriptors.

In terms of what to do next, I'd suggest posting your ideas for the next steps to the dev list.