Bug 49045

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?

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.

(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

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

*** Bug 49062 has been marked as a duplicate of this bug. ***

Ping. When do you expect to have an updated patch?

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.

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.