You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by Kevan Miller <ke...@gmail.com> on 2005/11/16 17:44:30 UTC
BasicProxyManager.java commit r344848
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.
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...
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 <am...@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);
>
>
>
Re: BasicProxyManager.java commit r344848
Posted by Kevan Miller <ke...@gmail.com>.
I'm good with that...
--kevan
On 11/17/05, Dain Sundstrom <da...@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 <da...@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 <ke...@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 <am...@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);
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> >
> >
>
>
Re: BasicProxyManager.java commit r344848
Posted by Aaron Mulder <am...@alumni.princeton.edu>.
Sure, that would be great.
Aaron
On 11/17/05, Dain Sundstrom <da...@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 <da...@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 <ke...@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 <am...@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);
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> >
> >
>
>
Re: BasicProxyManager.java commit r344848
Posted by Dain Sundstrom <da...@iq80.com>.
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 <da...@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 <ke...@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 <am...@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);
> >>>
> >>>
> >>>
> >>
> >>
>
>
Re: BasicProxyManager.java commit r344848
Posted by Kevan Miller <ke...@gmail.com>.
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 <da...@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 <ke...@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 <am...@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);
> >>>
> >>>
> >>>
> >>
> >>
>
>
Re: BasicProxyManager.java commit r344848
Posted by Aaron Mulder <am...@alumni.princeton.edu>.
Hmm. In the interesting case, we already pass in the CL to the BPM,
but then it has some internal method that it just passes the clases
to, and that's the one that creates the Enhancer and sets the CL on
that. I guess we just need to pass the original CL through. That
sure seems obvious now that you mention it. :)
Aaron
On 11/17/05, Dain Sundstrom <da...@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 <ke...@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 <am...@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);
> >>>
> >>>
> >>>
> >>
> >>
>
>
Re: BasicProxyManager.java commit r344848
Posted by Dain Sundstrom <da...@iq80.com>.
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 <ke...@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 <am...@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);
>>>
>>>
>>>
>>
>>
Re: BasicProxyManager.java commit r344848
Posted by Aaron Mulder <am...@alumni.princeton.edu>.
On 11/16/05, Kevan Miller <ke...@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 <am...@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);
> >
> >
> >
>
>