Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Hi,

I came across a problem related to class loading in SCR and before 
trying to fix this I would get some feedback if this is really a bug 
(see FELX-624).

Now, the problem is as follows - we have three bundles.
Bundle C : Provides a service for SERVICE_X which is registered through SCR.

Bundle A : Provides an abstract class AbstractClass that has a bind and 
unbind method for SERVICE_X. Therefore bundle A imports this service.

Bundle B : Provides a component COMPONENT_Y through SCR which inherits 
from AbstractClass from Bundle A. The class for COMPONENT_Y does nothing 
by itself with SERVICE_X, so there is no import and no import statement 
required for bundle B for SERVICE_X.

All bundles can be deployed and resolved, but when starting bundle B, 
there is a ClassNotFoundException as the SCR implementation searches for 
a bind method with SERVICE_X as the parameter. The SCR implementation 
(DependencyManager#734) uses the classloader from bundle B to load the 
parameter class. In this case this is SERVICE_X which is not visible for 
the classloader of bundle B.

So the question is, should there be a more clever search algorithm to 
resolve the parameter class? Or is this such an obscure use case that 
rather the "workaround" of adding an import statement to SERVICE_X in 
bundle B should be used?

Please note that we ran into this problem several times in the past, but 
never had the time to follow up on it and always used the workaround. We 
work with several abstract classes providing basic service functionality 
comming from a "common" bundle.

Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Felix Meschberger wrote:

> 
> Nevertheless it looks better to me to find a method with the interface 
> class known to the consumer (Component C) and get a ClassCastException 
> in case the Class objects would not match than to no be able to find the 
> bind method due to incompatible class objects.
> 
Agreed, your solution creates the more correct error message, so +1 :)

Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

[Felix Meschberger <fm...@gmail.com>]

Hi,

THe point is actually, that in an OSGi framework multiple bundles might 
be exporting the same package, yet with different versions. The Bundle A 
with Component C referring to service interface I (interface exported by 
Bundles B and D) might use a different interface class object than the 
actual Service S provided by Bundle E.

Here, Bundle A might import the interface I from Bundle B while Bundle E 
might import the service from Bundle D. Thus, the service may actually 
not be bound to Component C.

Of course the service registry of the OSGi framework ensures that 
Service S (registered with interface I imported from Bundle D) is not 
provided to Component C referring to the interface I imported from Bundle B.

Nevertheless it looks better to me to find a method with the interface 
class known to the consumer (Component C) and get a ClassCastException 
in case the Class objects would not match than to no be able to find the 
bind method due to incompatible class objects.

YMMV of course ...

Regards
Felix

Carsten Ziegeler schrieb:
> Felix Meschberger wrote:
>>
>> Right, this should work as the framework should already guarantee that 
>> the class object used by the service and the class object used by the 
>> abstract class with the bind methods are the same.
>>
>> Nevertheless, I would think, that using the class loader from 
>> Class.getClassLoader() of the class providing the bind (or unbind) 
>> method to be called is probably better than using the bundle providing 
>> the service. Its just cleaner IMHO.
>>
>> That is the assignment of the parameterClass in DependencyManager#734 
>> would be:
>>
>>    parameterClass =
>>        targetClass.getClassLoader().loadClass(parameterClassName)
>>
>> instead of using serviceBundle.loadClass.
>>
>> Of course, we would have to catch (and ignore) the 
>> ClassNotFoundException here, as the loader of the target class might 
>> not have the parameter class because the target class does not have 
>> the bind method we are looking for.
>>
> Hmm, not sure if this is cleaner :) In the end the class loader which 
> exposes
> the service interface is used as this one is exporting the interface. So 
> as we already have exactly this class from this class loader its easier 
> (and faster?) to use this directly than to refetch this.
> I guess in the end it's a matter of taste. I have no real preference as 
> long as its working :)
> 
> Carsten

Re: Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Felix Meschberger wrote:
> 
> Right, this should work as the framework should already guarantee that 
> the class object used by the service and the class object used by the 
> abstract class with the bind methods are the same.
> 
> Nevertheless, I would think, that using the class loader from 
> Class.getClassLoader() of the class providing the bind (or unbind) 
> method to be called is probably better than using the bundle providing 
> the service. Its just cleaner IMHO.
> 
> That is the assignment of the parameterClass in DependencyManager#734 
> would be:
> 
>    parameterClass =
>        targetClass.getClassLoader().loadClass(parameterClassName)
> 
> instead of using serviceBundle.loadClass.
> 
> Of course, we would have to catch (and ignore) the 
> ClassNotFoundException here, as the loader of the target class might not 
> have the parameter class because the target class does not have the bind 
> method we are looking for.
> 
Hmm, not sure if this is cleaner :) In the end the class loader which 
exposes
the service interface is used as this one is exporting the interface. So 
as we already have exactly this class from this class loader its easier 
(and faster?) to use this directly than to refetch this.
I guess in the end it's a matter of taste. I have no real preference as 
long as its working :)

Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

[Felix Meschberger <fm...@gmail.com>]

Hi,

Carsten Ziegeler schrieb:
> Stuart McCulloch wrote:
>>
>> looking at the current code that trawls the super-classes, I believe the
>> problem could be solved by changing 'DependencyManager.java:734'
>> so a ClassNotFoundException will drop down to the exhaustive search
>> of declared methods (this code would need to be refactored out, so it
>> can be called from multiple catch blocks)

The reason for trawling the super-classes is, that protected methods are 
only accessible through reflection with the Class.getDeclaredMethod 
method. This method in turn only returned methods of the Class itself 
not methods of extended classes.

>>
>> re-writing that whole function might reveal more optimizations though :)
>>
> Yes, I took a much simpler approach which should be alright: I use the 
> bundle which exports the service for loading the class. This classloader 
> has already loaded the class as the bundle is exporting this service. 
> And this class should be the same which should be used for the 
> bind/unbind method as otherwise class cast exceptions would occur.

Right, this should work as the framework should already guarantee that 
the class object used by the service and the class object used by the 
abstract class with the bind methods are the same.

Nevertheless, I would think, that using the class loader from 
Class.getClassLoader() of the class providing the bind (or unbind) 
method to be called is probably better than using the bundle providing 
the service. Its just cleaner IMHO.

That is the assignment of the parameterClass in DependencyManager#734 
would be:

    parameterClass =
        targetClass.getClassLoader().loadClass(parameterClassName)

instead of using serviceBundle.loadClass.

Of course, we would have to catch (and ignore) the 
ClassNotFoundException here, as the loader of the target class might not 
have the parameter class because the target class does not have the bind 
method we are looking for.

WDYT ?

Regards
Felix

> 
> A quick test of our hugh application massivly using SCR did not reveal 
> any problems.
> 
> Carsten
> 

Re: Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Stuart McCulloch wrote:
> 
> looking at the current code that trawls the super-classes, I believe the
> problem could be solved by changing 'DependencyManager.java:734'
> so a ClassNotFoundException will drop down to the exhaustive search
> of declared methods (this code would need to be refactored out, so it
> can be called from multiple catch blocks)
> 
> re-writing that whole function might reveal more optimizations though :)
> 
Yes, I took a much simpler approach which should be alright: I use the 
bundle which exports the service for loading the class. This classloader 
has already loaded the class as the bundle is exporting this service. 
And this class should be the same which should be used for the 
bind/unbind method as otherwise class cast exceptions would occur.

A quick test of our hugh application massivly using SCR did not reveal 
any problems.

Carsten

-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

[Stuart McCulloch <st...@jayway.net>]

2008/7/7 Carsten Ziegeler <cz...@apache.org>:

> Stuart McCulloch wrote:
>
>>
>> imho SCR should be using the classloader of the class that declared the
>> bind/unbind method, ie:
>>
>>   method.getDeclaringClass().getClassLoader()
>>
>> when loading parameter classes - always using B's classloader doesn't
>> sound
>> right to me
>>
>>  Yes, I agree - now, the current implementation uses the classname of the
> service (string) to load the parameter class and then this class to find the
> corresponding method.
>
> We could change this to iterate over all available methods and pick the one
> with the correct parameter (by string comparision) or - and this seems
> easier to me - use directly the class of the service. The bind/unbind
> methods are searched with a valid ServiceReference. This reference can be
> used directly to get the service itself and the service class.
>
> WDYT?
>

looking at the current code that trawls the super-classes, I believe the
problem could be solved by changing 'DependencyManager.java:734'
so a ClassNotFoundException will drop down to the exhaustive search
of declared methods (this code would need to be refactored out, so it
can be called from multiple catch blocks)

re-writing that whole function might reveal more optimizations though :)

Thanks
>
> Carsten
> --
> Carsten Ziegeler
> cziegeler@apache.org
>

-- 
Cheers, Stuart

Re: Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Richard S. Hall wrote:
> 
> It probably makes sense to change this in some fashion. We have the same 
> issue inside the framework when we are trying to figure out how if a 
> class is visible to a given bundle (in Util.loadClassUsingClass()). So, 
> it makes sense that SCR might have to do something similar.
> 
Ah, yes, thanks for the pointer Richard. This should do the trick.

However, I have the feeling that we don't need this as the service class 
is already loaded and available through the ServiceReference. I have to 
dig a little bit more into the code ...or I could make my life easier 
and use code similar to Util.loadClassUsingClass :)

Thanks!
Carsten

-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

["Richard S. Hall" <he...@ungoverned.org>]

Carsten Ziegeler wrote:
> Stuart McCulloch wrote:
>>
>> imho SCR should be using the classloader of the class that declared the
>> bind/unbind method, ie:
>>
>>    method.getDeclaringClass().getClassLoader()
>>
>> when loading parameter classes - always using B's classloader doesn't 
>> sound
>> right to me
>>
> Yes, I agree - now, the current implementation uses the classname of 
> the service (string) to load the parameter class and then this class 
> to find the corresponding method.
>
> We could change this to iterate over all available methods and pick 
> the one with the correct parameter (by string comparision) or - and 
> this seems easier to me - use directly the class of the service. The 
> bind/unbind methods are searched with a valid ServiceReference. This 
> reference can be used directly to get the service itself and the 
> service class.
>
> WDYT?

It probably makes sense to change this in some fashion. We have the same 
issue inside the framework when we are trying to figure out how if a 
class is visible to a given bundle (in Util.loadClassUsingClass()). So, 
it makes sense that SCR might have to do something similar.

-> richard

>
>
> Thanks
> Carsten

Re: Question about class loading and SCR (see FELIX-624)

[Carsten Ziegeler <cz...@apache.org>]

Stuart McCulloch wrote:
> 
> imho SCR should be using the classloader of the class that declared the
> bind/unbind method, ie:
> 
>    method.getDeclaringClass().getClassLoader()
> 
> when loading parameter classes - always using B's classloader doesn't sound
> right to me
> 
Yes, I agree - now, the current implementation uses the classname of the 
service (string) to load the parameter class and then this class to find 
the corresponding method.

We could change this to iterate over all available methods and pick the 
one with the correct parameter (by string comparision) or - and this 
seems easier to me - use directly the class of the service. The 
bind/unbind methods are searched with a valid ServiceReference. This 
reference can be used directly to get the service itself and the service 
class.

WDYT?


Thanks
Carsten
-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Question about class loading and SCR (see FELIX-624)

[Stuart McCulloch <st...@jayway.net>]

2008/7/7 Carsten Ziegeler <cz...@apache.org>:

> Hi,
>
> I came across a problem related to class loading in SCR and before trying
> to fix this I would get some feedback if this is really a bug (see
> FELX-624).
>
> Now, the problem is as follows - we have three bundles.
> Bundle C : Provides a service for SERVICE_X which is registered through
> SCR.
>
> Bundle A : Provides an abstract class AbstractClass that has a bind and
> unbind method for SERVICE_X. Therefore bundle A imports this service.
>
> Bundle B : Provides a component COMPONENT_Y through SCR which inherits from
> AbstractClass from Bundle A. The class for COMPONENT_Y does nothing by
> itself with SERVICE_X, so there is no import and no import statement
> required for bundle B for SERVICE_X.
>
> All bundles can be deployed and resolved, but when starting bundle B, there
> is a ClassNotFoundException as the SCR implementation searches for a bind
> method with SERVICE_X as the parameter. The SCR implementation
> (DependencyManager#734) uses the classloader from bundle B to load the
> parameter class. In this case this is SERVICE_X which is not visible for the
> classloader of bundle B.
>
> So the question is, should there be a more clever search algorithm to
> resolve the parameter class? Or is this such an obscure use case that rather
> the "workaround" of adding an import statement to SERVICE_X in bundle B
> should be used?
>

imho SCR should be using the classloader of the class that declared the
bind/unbind method, ie:

   method.getDeclaringClass().getClassLoader()

when loading parameter classes - always using B's classloader doesn't sound
right to me

Please note that we ran into this problem several times in the past, but
> never had the time to follow up on it and always used the workaround. We
> work with several abstract classes providing basic service functionality
> comming from a "common" bundle.
>
> Carsten
> --
> Carsten Ziegeler
> cziegeler@apache.org
>



-- 
Cheers, Stuart