geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dain Sundstrom <d...@iq80.com>
Subject Re: BasicProxyManager.java commit r344848
Date Thu, 17 Nov 2005 21:56:12 GMT
I can make the changes if you guys want.

-dain

On Nov 17, 2005, at 12:55 PM, Kevan Miller wrote:

> Dain,
> This is regarding a fix for http://issues.apache.org/jira/browse/ 
> GERONIMO-1064
>
> The problem, which Aaron diagnosed, is that CGLIB will use the  
> ClassLoader of the first class/interface that we pass it (unless we  
> explicitly set the ClassLoader). If one of the subsequent  
> interfaces is not loadable using this ClassLoader, the enhancer  
> will fail.
>
> Aaron's fix addressed this problem by searching for a ClassLoader  
> which could load all of the classes.
>
> I don't have much experience with BasicProxyManager, but your  
> proposal seems much cleaner than either Aaron's or my fix for the  
> problem and I think it addresses shortcomings in the current  
> interface...
>
> --kevan
>
> On 11/17/05, Dain Sundstrom <dain@iq80.com> wrote: I'm confused why  
> we are trying guess the parent most class loader....
>
> When we are creating a proxy to a service, our goal is to create a
> class that the caller can interact with.  This mean that no matter
> what interfaces we would like the proxy to implement, all of those
> interfaces must be loaded from the caller's class loader.  Since the
> caller knows its own class loader, why not require the the caller
> pass in its class loader?
>
> I think we should always use this form of createProxy:
>
>      public Object createProxy(ObjectName target, ClassLoader loader);
>
> or a new form like this, which would grab the thread context  
> classloader
>
>      public Object createProxy(ObjectName target);
>
> and we deprecate the others.  This version is much cleaner because it
> load all interface class directly from the specified class loader
> which eliminates the possibility of having a class from the wrong
> class loader.
>
> The other problem with BasicProxyManager is the method
> createProxyFactory does not accept a target class loader.  This
> should be a mandatory argument to the method.
>
> -dain
>
> On Nov 16, 2005, at 8:54 AM, Aaron Mulder wrote:
>
> > On 11/16/05, Kevan Miller <kevan.miller@gmail.com> wrote:
> >> Aaron,
> >>  Nice work getting to the bottom of this issue. However, I'm not
> >> sure that
> >> I'm happy with your fix. I'm confident that your fix will find a
> >> ClassLoaders which can load all of the classes/interfaces.
> >> However, there
> >> can be multiple of these and there's no guarantee that you're
> >> finding the
> >> most appropriate ClassLoader. For example imagine an application
> >> classloader
> >> with inverseClassLoading set to true. You're technique might find
> >> a parent
> >> ClassLoader, when the desired ClassLoader is the application
> >> classloader.
> >
> > For my part, I don't really care which class loader is "ideal" so  
> long
> > as we try to get one that can load all the classes.  I'm not too
> > concerned that parent and child may be using different  
> definitions of
> > the same class such that a different "master" CL might result in  
> using
> > a different definition.  We're only talking about GBean interfaces
> > here, and I think unlikely to have different versions in use by
> > different CLs (at present).  Can you think of a specific use case in
> > Geronimo where we might run into a problem?  I know David J is  
> working
> > on fully versioned configurations, which might make a difference.
> >
> >>  I have an alternate fix which calculates a child ClassLoader from
> >> the
> >> potential list of ClassLoaders (my fix assumes that there is one
> >> ClassLoader
> >> to which all other ClassLoaders are ancestors). I've tested
> >> against Joe's
> >> test case. It too fixes the problem...
> >>
> >>  As I'm typing this, I'm wondering if we have an even larger
> >> problem. Is
> >> there a guarantee that the list of ClassLoaders available to the
> >> BasicProxyManager constructor contains a single ClassLoader which
> >> is capable
> >> of loading <all> of the given classes? Seems pretty easy to
> >> construct a
> >> scenario in which this is not true. I'd be interested in hearing
> >> what you or
> >> others might think...
> >
> > Right, there's no guarantee this will solve all possible problems.
> > Right now it just falls through if it can't identify a "master"  
> CL.  I
> > suppose we could create a new multi-parent CL on the spot if we  
> needed
> > to.
> >
> > Aaron
> >
> >>  A problematic scenario would look like this:
> >>
> >>         System ClassLoader
> >>              /      |     \
> >>             A     B     C
> >>               \     |     /
> >>            My ClassLoader
> >>
> >>  Assume that ClassLoader A is the loader for class a, etc. If the
> >> classes a,
> >> b, and c are passed to the BasicProxyManager constructor, it will
> >> not be
> >> able to determine a ClassLoader capable of loading a, b, <and> c.
> >> Is this an
> >> invalid use case? Neither of our fixes will work in this case...
> >>
> >>  --kevan
> >>
> >>
> >> On 11/15/05, ammulder@apache.org <ammulder@apache.org> wrote:
> >>> Author: ammulder
> >>> Date: Tue Nov 15 18:32:31 2005
> >>> New Revision: 344848
> >>>
> >>> URL: http://svn.apache.org/viewcvs?rev=344848&view=rev
> >>> Log:
> >>> Pick the best ClassLoader for the provided set of interfaces
> >>>   (Fixes GERONIMO-1064)
> >>>
> >>> Modified:
> >>>
> >> geronimo/trunk/modules/kernel/src/java/org/apache/geronimo/kernel/
> >> basic/BasicProxyManager.java
> >>>
> >>> Modified:
> >> geronimo/trunk/modules/kernel/src/java/org/apache/geronimo/kernel/
> >> basic/BasicProxyManager.java
> >>> URL:
> >> http://svn.apache.org/viewcvs/geronimo/trunk/modules/kernel/src/
> >> java/org/apache/geronimo/kernel/basic/BasicProxyManager.java?
> >> rev=344848&r1=344847&r2=344848&view=diff
> >>>
> >>  
> =====================================================================
> >> =========
> >>> ---
> >> geronimo/trunk/modules/kernel/src/java/org/apache/geronimo/kernel/
> >> basic/BasicProxyManager.java
> >> (original)
> >>> +++
> >> geronimo/trunk/modules/kernel/src/java/org/apache/geronimo/kernel/
> >> basic/BasicProxyManager.java
> >> Tue Nov 15 18:32:31 2005
> >>> @@ -207,6 +207,24 @@
> >>>              } else if(type.length == 1) { // Unlikely (as a
> >>> result of
> >> GeronimoManagedBean)
> >>>                  enhancer.setSuperclass(type[0]);
> >>>              } else {
> >>> +                ClassLoader best = null;
> >>> +                outer:
> >>> +                for (int i = 0; i < type.length; i++) {
> >>> +                    ClassLoader test =
> >> type[i].getClassLoader();
> >>> +                    for (int j = 0; j < type.length; j++) {
> >>> +                        String className = type[j].getName();
> >>> +                        try {
> >>> +                            test.loadClass(className);
> >>> +                        } catch (ClassNotFoundException e) {
> >>> +                            continue outer;
> >>> +                        }
> >>> +                    }
> >>> +                    best = test;
> >>> +                    break;
> >>> +                }
> >>> +                if(best != null) {
> >>> +                    enhancer.setClassLoader(best);
> >>> +                }
> >>>                  if(type[0].isInterface()) {
> >>>                      enhancer.setSuperclass(Object.class);
> >>>                      enhancer.setInterfaces(type);
> >>>
> >>>
> >>>
> >>
> >>
>
>


Mime
View raw message