geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Mulder <ammul...@alumni.princeton.edu>
Subject Re: BasicProxyManager.java commit r344848
Date Thu, 17 Nov 2005 22:17:49 GMT
Sure, that would be great.

Aaron

On 11/17/05, Dain Sundstrom <dain@iq80.com> wrote:
> 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