Reimplementing AspectManager/Domains to understand jboss-cl

[kabirkhan]

I have almost completed the reimplementation of the classpools
http://www.jboss.org/index.html?module=bb&op=viewtopic&t=143639
To understand the classloading rules used in JBoss AOP, all that is outstanding is a bit of work in AS 5 which will be possible following the upgrade to jboss-cl 2.0.3.GA. The next part is to reimplement the AspectManagers and Domains that are used in AOP to understand the classloading rules.

Before I start, here is how the classloader scoping currently works. It works by classloader domain, and the deployment containing AOP information determines where the AOP information is deployed. For woven classes, the classloader domain of its loading class determines which AOP information is applied to that class.

Currently, if a deployment is not scoped (i.e. the classloader belongs to the default domain) the AOP information is deployed to the main AspectManager. The AOP information used is shared among all classes loaded by classloaders belonging to the default domain. If a deployment is scoped (i.e. the classloader belongs to a child of the default domain) the AOP information is deployed to an AOP Domain, which is mapped onto the classloader Domain. Again, all the information deployed into that AOP Domain is applied to all classes loaded by classloaders belonging to the corresponding classloader Domain. Domains are hierarchical with a parent link that takes you to the parent Domain, with the "root" the main AspectManager. A child domain will inherit AOP information from its parents in this scenario with parent-first/-last ordering controlled by the behaviour of the classloader Domain.

In a nutshell, everything is visible to everything else that belongs to that classloader domain, which is fine at the moment since to my knowledge nobody is really using the more advanced features of the classloaders in AS 5 yet.

With the new classloaders there can be some visibility constraints between classloaders deployed within a domain, making us need some way to define what AOP information is visble between different loaders.

There are a few main use-cases and scenarios in which jboss aop is deployed, I will try to outline some problems I can see at the moment wrt how these are deployed, assuming that there are two classloaders deployed to a classloader domain, where:

ClassLoader A:
Imports: pkg.b
Private packages: pkg.a

ClassLoader B:
Exports: pkg.b
ImportAll: false

The main problem I can see is with AOP "introducing" new classes to the woven POJOs. In the examples given the new classes are from added interceptors, but the same holds true for interface introductions, mixins, annotation-introductions, metadata-loaders etc.

1) AOP deployed as part of a standalone jboss-aop.xml
=====================================================
Assuming that the classes have been prepared, an -aop.xml file is deployed containing binding information that is used to apply bindings to the classes. What happens is

a) New class is visible from the affected classes classloader

<bind pointcut="all(pkg.a.PojoA) OR all(pkg.b.PojoB)">
 <interceptor name="pkg.b.MyInterceptor"/>
</bind>

This should work out of the box since CL B has the pkg.b.MyInterceptor class , and it can be seen from both PojoA's loader (CL A) and PojoB's loader (CL B).

b) New class is not visible from the affected classes classloader

<bind pointcut="all(pkg.a.PojoA) OR all(pkg.b.PojoB)">
 <interceptor name="pkg.a.MyInterceptor"/>
</bind>

Here PojoA would get the extra interceptor, PojoB's would currently cause an error since pkg.a.MyInterceptor cannot be loaded from CL B.


2) AOP deployed as part of a deployment
=======================================
An .aop file is deployed containing a META-INF/jboss-aop.xml file that is used to apply bindings to the classes. Normally, this should contain the interceptor classes (discussed below) , but could also refer to external classes (which is discussed in point 1 above)

a) New class is visible from the affected classes classloader
Deployment against CL B contains a .aop file containing the following

<bind pointcut="all(pkg.a.PojoA) OR all(pkg.b.PojoB)">
 <interceptor name="pkg.b.MyInterceptor"/>
</bind>

This will work since pkg.b.MyInterceptor is visible from both CL A and CL B.

b) New class is not visible from the affected classes classloader
Deployment against CL B contains a .aop file containing the following

<bind pointcut="all(pkg.a.PojoA) OR all(pkg.b.PojoB)">
 <interceptor name="pkg.a.MyInterceptor"/>
</bind>

Here PojoA would get the extra interceptor, PojoB's would currently cause an error since pkg.a.MyInterceptor cannot be loaded from CL B.

c) New standalone class is not visible from the affected classes classloader
If I deploy a c.aop on its own that gets the classloader CL C deployed against the same domain as A and B, containing

<bind pointcut"all(pkg.a.PojoA) OR all(pkg.b.PojoA)">
 <!-- loaded by classloader C, part of c.aop -->
 <interceptor name="pkg.c.MyInterceptor"/>
</bind>

As far as I can tell, neither CL A nor CL B will be able to load pkg.c.MyInterceptor?


I am slightly unsure what happens when the classes are woven in the examples given. When weaving PojoA, that gets additional interfaces such as the the org.jboss.aop.Advised interface, references to org.jboss.aop.Advisor etc., which I am not sure off the top of my head are visible from CL A?

In the examples I gave, JBoss AOP will currently throw an error if a required interceptor class cannot be found. A few ideas to fix this is

I) Swallow Exceptions
If an interceptor class or introduced interface cannot be found, just log the error and continue. I don't think that is a good thing though, but included it for completeness.

II) Use a special classloader for interceptors etc.
A special aop classloader could be used for the aspects at runtime, but that would require changes to the woven code to set the correct classloader, which we don't really want to do when weaving/running outside of the AS. Also, as in the cases of interface introductions the interface/mixin must be visible when the class is loaded.

III) Somehow make new classes from AOP globally visible
How?

IV) Set up delegating AOP domains for each class(loader)
Instead of giving a reference to the whole AOP Domain when getting the domain for a classloader, it should return a AOPClassLoaderManager that is unique to that classloader, and contains the things from the underlying AOP Domain that are visible from the classloader. For example in 2b) above the AOPClassLoaderManager for CL A would contain the binding, while the AOPClassLoaderManager for CL B would not contain that particular binding. I am unsure how that would work for 2c) if pkg.c.MyInterceptor is not visible from CL A or B? While this might be another flavour of "Swallow Exceptions", at least we could include some management capability to see what exists in the classloading domain vs what is visible from the classloader. As to how to use these rules in the domains, we were able to delegate a lot of work for the classloading rules to jboss-cl. Maybe the easiest is to do the same here as well? When adding a binding or something else that needs to load a class make sure that the AOPClassLoaderManager is able to load up that class using its classloader, and if not blacklist that binding? When a classloader joins the classloader Domain later, recheck all the AOP Domain's blacklisted bindings etc. and unblacklist the newly found ones?

[kabirkhan]

Coming back to this IV is not a good solution. As far as I can tell it means that you can never have "external" aspects (aspects in packages not declared in the loaders import or the loader itself) applied to classes in classloaders using importAll=false. I think we need a way to make it possible to have aspects deployed globally and applied to loaders using importAll=false, but maybe in reality it isn't as important as I think?

I'm just thinking loud, hopefully that will help me organise my thoughts later...

A simple solution would be to include the packages of aspects that might be applied in the target classloader's optional imports. But that might be a step in the wrong direction, limiting the scope of dynamic aop?

For II and III we need some way to identify classloaders that contain aspects. That could be done if we make it a requirement that aspects which should apply to more than one deployment are deployed in a top-level .aop archive. Nested .aop archives could only apply to that deployment if import/export is used, but again the bindings contained in its jboss-aop.xml might be referencing aspects from outside that loader?

If we have a way to identify aspect deployments something needs doing to make the classes globally visible within the domain. A few initial ideas:
-Have "special" classloaders within the domain for aspects which are checked even by classloaders using importAll=false
-Have AOP stuff deployed in a parent domain (if this is actually checked when importAll=false) of the real domain, something like:

AOPDefaultDomain
 DefaultDomain
 AOPChildDomain1
 ChildDomain1
 AOPChildDomain2
 ChildDomain2

[adrian.brock]

I'm not following what problem you are trying to solve?

If a class isn't visible using the classloader rules then you shouldn't be able
to use AOP to change that.

e.g. In your final comment where you want a "global" aspect definition then
the deployments that want to use it would need to "import" the classloader
where those aspects are deployed.

I thought the issue for you was just how you attach the aop domains to the
shared classloader spaces (either the old import-all rules or imports).
Doesn't the getClassLoaderFor() stuff solve this?

if the issue is mainly an ease-of-use thing, i.e. allowing people to share
config where part of the config contains classes that are not visible but are
irrelevant then that's really your choice.

e.g. Do you still allow this to work where pkg.a is invisible.
You could ignore the fact that PojoA cannot be loaded from a deployment because
it is not relevant to an application using pkg.b?

<bind pointcut="all(pkg.a.PojoA) OR all(pkg.b.PojoB)">
 <interceptor name="pkg.b.MyInterceptor"/>
</bind>

[kabirkhan]

The problem I am trying to solve is not really to do with the classes being woven, rather with being able to load up bound aspects when invoking the woven class. If the aspects are part of the the target deployment it is not an issue. There will be a problem if the aspects are in other deployments and the aspect classes are not found by the target deployment's classloader.

For example if I have a deployment A.jar using importAll=false containing

package pkga;

@Marker
public class ClassA{
 public void method(){}
}

In same the classloading domain I have a B.aop archive containing pkgb.InterceptorB.class:

<aop>
 <interceptor class="pkgb.InterceptorB"/>
 <bind pointcut="execution(* @Marker->*(..))">
 <interceptor-ref name="pkgb.InterceptorB"/>
 </aspect>
</aop>

and a C.aop archive containing pkgc.InterceptorC.class:

<aop>
 <interceptor class="pkgc.InterceptorC"/>
 <bind pointcut="execution(* @Marker->*(..))">
 <interceptor-ref name="pkgc.InterceptorC"/>
 </aspect>
</aop>

When the class is loaded/woven we don't care about what is bound, so we can weave the class no problem.

When we come to invoking pkga.ClassA.method(), we need to populate the interceptor chains. Since A.jar's loader is not set up to see pkgb.InterceptorB or pkgc.InterceptorC we can't invoke those interceptors. Currently AOP will throw an error, the purpose of this thread is to figure out a better way to handle this.

e.g. In your final comment where you want a "global" aspect definition then
the deployments that want to use it would need to "import" the classloader
where those aspects are deployed.

I think you are saying just ignore InterceptorB and InterceptorC unless A.jar is set up to import the packages/modules of the interceptors? I think that will be pretty simple to do.

My suggestion regarding global visibility is to make the classes from the loaders for B.aop and C.aop magically visible from A.jar somehow. But that means we don't get versioning of modules packages, so maybe an explicit import is the best thing. At the same time, I don't know if whoever writes A.jar should need to know that InterceptorB or InterceptorB might be applied later?

When generating the invocation class for pkga.ClassA.method() we create something like:

public class ClassA_method extends MethodInvocation
{
 ClassA target;

 public ClassA_method(POJO target)
 {
 this.target = target;
 }

 public void dispatchJoinPoint()
 {
 target.method();
 }
}

public class ClassA_method_1 extends ClassA_method
{
 pkgb.InterceptorB aspect1;
 pkgc.InterceptorC aspect2;

 int current;
 public Object invokeNext() throws Throwable
 {
 try
 {
 switch (current++)
 {
 case 0:
 return aspect1.invoke(this);
 break;
 case 1:
 return aspect2.invoke(this);
 break;
 default:
 dispatchJoinPoint();
 return null;
 }
 }
 finally
 {
 current--;
 }
 }
}

If go with ignoring the aspects that cannot be found we would need to generate this instead:

public class ClassA_method_1 extends ClassA_method
{
 int current;
 public Object invokeNext() throws Throwable
 {
 switch (current++)
 {
 case 0:
 return aspect1.invoke(this);
 break;
 case 1:
 return aspect2.invoke(this);
 break;
 default:
 dispatchJoinPoint();
 return null;
 }
 }
}

[adrian.brock]

e.g. In your final comment where you want a "global" aspect definition then
the deployments that want to use it would need to "import" the classloader
where those aspects are deployed.

I think you are saying just ignore InterceptorB and InterceptorC unless A.jar is set up to import the packages/modules of the interceptors? I think that will be pretty simple to do.

It depends what you are talking about. If the aspect definition is in A.jar
but the class is invisible then I think it should be an error.

I don't see why aspects shouldn't follow the same rules as normal class imports?
The fact that AOP is weaving the class reference into bytecode
(either literally or dynamically via interceptors) shouldn't make any difference.

My suggestion regarding global visibility is to make the classes from the loaders for B.aop and C.aop magically visible from A.jar somehow. But that means we don't get versioning of modules packages, so maybe an explicit import is the best thing. At the same time, I don't know if whoever writes A.jar should need to know that InterceptorB or InterceptorB might be applied later?

Magically adding a random version of the class is clearly not in sympathy with
the modularization rules of OSGi style classloading.

The issue that I said was upto you is whether InterceptorB defined in B.aop really applies
to A.jar when everything else from B.aop is not visible to it.

I'd say the rule should be that the aop config only applies when A.jar chooses
to import the relevant aspect classes defined in B.aop.

The part I said you might ignore would be where B.aop contains aspect
configuration for other classes that A.jar chooses NOT to import or just isn't relevant.

I don't know whether that helps you resolve the problem, but you certainly shouldn't
be trying to break the class visbility rules in any way. That could be a potential
security hole if nothing else. :-)

[adrian.brock]

Probably the easiest compromise is just log a warning, e.g.

WARN Not apply aspect B to com.acme.Blah because b.BInterceptor cannot be
loaded from classloader A.jar

But some might think that is serious problem if is B is really the security aspect
and b.BInterceptor is really the Authentication interceptor. :-)
i.e. "Blah" would not have the intended security aspect.

[kabirkhan]

Probably the easiest compromise is just log a warning, e.g.

Agreed

But some might think that is serious problem if is B is really the security aspect
and b.BInterceptor is really the Authentication interceptor. :-)
i.e. "Blah" would not have the intended security aspect.

At least we are logging it, and they can fix it by adding an import to the affected target archive.

Thanks!

[jason.greene]

I agree with Adrian's position regarding classloading. We should never violate the classloading configuration. That leads to unexpected and unpredictable behavior.

If we feel the urge to add "magic" here, then we are probably doing something wrong.