AOP asintegration WITHOUT the integration :-)

[adrian.brock]

Related to these threads:

New classloader tests - http://www.jboss.com/index.html?module=bb&op=viewtopic&t=114427
Refactoring ASIntegration for new classloader -
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=112771
AOP Scoping is WRONG!
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=112736

I think I've found a way to make this work without having to any integration.
The basic problem is the one I found with the TCL usage.
When this is fixed (and some other things), we don't need any integration code.

The new tests I wrote now all work with the following two changes:

1) In JoinPointGenerator I now always use the classloader of the pool
where the advice lives, not the TCL.
2) I've added a new option "pushClassLoader" to the AspectManager
which says that when generating advices it should be done under the
classloader of the **Advised** class

Obviously these changes are incomplete:
a) Part (2) above should be using the classloader of the **Advice** class
b) I'm only doing (2) for PER_VM and PER_CLASS since that's all I've tested so far
c) There are other places besides the Advisor that generate advices
d) There are a number of other places that use the TCL that need fixing
in a similar way.

There's one other fix that needs doing which is in (I haven't made this change)
AspectManager.findClassPool()

The use of Translatable is obsolete. It should really be doing something like:

if (cl == null)
 cl = ClassLoader.getSystemClassLoader(); // Using SecurityActions;
return registerClassLoader(cl);

For now, I've made RealClassLoader (which is what both the new and old
classloaders extend) implement Translatable.

[adrian.brock]

So, once this change (use the correct classloader instead of guessing the TCL)
is done throughout AOP there will be no need for ScopedClassLoaderDomains
or any other integration code beyond the javassist ScopedClassPool.

With just the few changes I've done already, all the tests I've written pass
including the ones that have unexported classes and multiple versions
of the classes which are some of the more complicated OSGi use cases.

There's one caveat which is that to make the multiple version test work,
I had to make the interceptor PER_CLASS scope.

The notion of PER_VM needs redefining such that it takes into account
multiple versions of the advice in different classloaders, but it should not have to
do complicated things like the ScopedClassLoaderDomain to do that.

The PER_VM advices should just be indexed by the classloader of the advice class.

[adrian.brock]

I've reverted the pushClassLoader change.

This was only working because the two different versions of the classes had the
same -aop.xml

I've updated the tests to show what the real problem is,
i.e. the pointcut matching doesn't understand the classloading.

[kabirkhan]

I've reverted the pushClassLoader change.
I've updated the tests to show what the real problem is,
i.e. the pointcut matching doesn't understand the classloading.

I've yet to completely understand your tests :-), but the point of the domain was to have different pointcuts/bindings deployed at different levels

[kabirkhan]

Could you point me towards some information on which way the imports/exports work? For example in the following, my interpretation is that the "A" classloader imports/uses all classes from the underlying domain, apart from if they belong to the "a" package?

public class SimpleImportAllUnitTestCase extends AOPIntegrationTest
{
 ...
 /*
 * A simple test that loads a class from another classloader
 * that uses aop enhanced classes from our classloader.
 *
 * The other classloader importsAll from the classloading system.
 */
 public void testImportAll() throws Exception
 {
 ClassLoader classLoader = createClassLoader("A", true, PACKAGE_A);
 try
 {
 Class<?> classA = classLoader.loadClass(CLASS_A);
 classA.newInstance();
 }
 finally
 {
 unregisterClassLoader(classLoader);
 }
 }
}

[adrian.brock]

The mock classloader isn't documented anywhere AFAIR,
I only created it for testing purposes.

The importAll and exportAll (not in the MockClassLoader)
are meant to duplicate what the old UCL does.
In the example you quote, it does do "importAll" so it will load from the whole
domain that the classloader belongs to.

The mock classloader has two important properties.

setPackages() - what is exported
setPaths() - what is part of the classloader (but not necessarily exported to other classloaders)

There is a convience method to do both, setPackagesAndPaths()

In the api you show, the "PACKAGE_A" is what is included in the classloader and
what is exported.

The "true" means it will "importAll", i.e. look at all other classloaders in the
order they were registered to find the class before looking at its own
private (non-exported) classes.

Try adding enableTrace("org.jboss.classloader") to the test
and you will see in detail where it is trying to load classes from
and where it found it.

[kabirkhan]

That certainly clears things up :-) In ComplexImportAdviceUnitTestCase, could you please explain the purpose of the

 MockClassLoaderPolicy supportPolicy = MockClassLoaderHelper.createMockClassLoaderPolicy("Support");
 supportPolicy.setPathsAndPackageNames(PACKAGE_SUPPORT);
 ClassLoader support = createClassLoader(supportPolicy);
 try
 {
 MockClassLoaderPolicy bPolicy = MockClassLoaderHelper.createMockClassLoaderPolicy("B");
 bPolicy.setPathsAndPackageNames(PACKAGE_B);
 bPolicy.setDelegates(createDelegates(supportPolicy));
 ClassLoader b = createClassLoader(bPolicy);

I am thinking of the following line:

bPolicy.setDelegates(createDelegates(supportPolicy));

[kabirkhan]

I've now had a look at the new aop classloading tests, and see the problem you have been describing.

So, the problem we have with the failing test is that it is trying to create the interceptor twice. This is what we solved before with the ScopedJBossClassPool/ScopedClassLoaderDomain. To summarize what you have probably said already with my own words, with the new classloaders, we don't really have scoped classloaders in this case. So the check to see if the classloader is scoped to create a new domain does not really work here... I see what you have done, which is basically to make sure that the correct classloader is used, however we are not really limiting what is available.

What is needed is maybe ***something along the lines of*** setting the classloader on the advicebindings (and other things) + a AdviceBinding.isVisble(ClassLoader) type method or something like that. I'll look more into this in the morning and hopefully come up with some better ideas :-)

[adrian.brock]

I am thinking of the following line:

bPolicy.setDelegates(createDelegates(supportPolicy));

This says the classloader "b" imports the classloader "support".
i.e. all the exported classes from "support" are visible to "b".

[kabirkhan]

With the multiple versions test-case, we are basically deploying two similar jboss-aop.xml files into the system. One goes into A1, the other into A2

 |---- Support1 -- A1
MOCK-
 |---- Support2 -- A2

I am not clear on what the visibility rules for resources are between the classloaders? Currently everything gets applied everywhere (in other words both in A1 and A2 we will get intercepted by TestInterceptor twice)
while it feels more natural that the jboss-aop.xml that is deployed into A1 only applies there, while the one that is depoyed into A2 only applies there. This is what the domains made easy :-) Of course we had the opposite case using the global domain where everything is deployed/visible everywhere. I guess I am trying to define this "partial" visiblity somehow wrt to the -aop.xml information.

Or should I not really care about this unless there is a "real" scope, i.e child domains, and just make sure that the AspectDefinitions have some indexing by classloader like you suggest?

[kabirkhan]

Or should I not really care about this unless there is a "real" scope, i.e child domains, and just make sure that the AspectDefinitions have some indexing by classloader like you suggest?

In other words, with the tests set up the way they are is it fine for the classes in A1 and A2 to get intercepted twice, just making sure that the the correct versions of the aspects are being used?

[adrian.brock]

To reproduce all the OSGi rules and old JBoss rules you would need
to look at the classloading rules and create AOP Domains that delegate to each other.

Think of A1, A2 and Support1/2 as applications. They are the
same application(s) deployed multiple times
(possibly different versions of the classes or just so the user can get their own singletons).

The AOPDeployer could use the classloading config and create domains
to match. i.e. A1 can see AOP configuration from A1 and Support1

For the old JBoss rules (importAll=true) then the visibilty would depend on the
domain name and parent policy like the old ScopedDomain.

The real work is going on in the "incomplete"
org.jboss.deployers.plugins.classloading.Module.

My preferred solution to use the scopes. i.e. you can choose
to deploy aop config at the SERVER, SUBSYSTEM, APPLICATION level, etc.
but this requires doing a lot work fixing everything to make sure
it sets the correct thread local MetaData for AOP to know which Scope
applies at runtime.

i.e. somebody could apply default aop config for some interception
at the server/global level and an application could override it.
Which AOP config applies when creating the object depends on the context of the
invocation NOT what classloader loads the aspect
(which could be in a different/global deployment) or the intercepted class (same reason)
to the application doing the work.

[adrian.brock]

My preferred solution to use the scopes. i.e. you can choose
to deploy aop config at the SERVER, SUBSYSTEM, APPLICATION level, etc.
but this requires doing a lot work fixing everything to make sure
it sets the correct thread local MetaData for AOP to know which Scope
applies at runtime.

This thead local is really just a generalization of ejbs/mbeans
setting the correct context classloader and java:/comp/env

In principle once this is done. The only work required by AOP
would be for the deployer to create hierarchical (same hierarchy as the scopes)
AOP Domains in the DeploymentUnit.get{Mutable}MetaData()
and deploy config into the correct scoped domain

The caveat being that it should be possible to override the scope,
e.g. a deployment could choose to deploy the aop confg into the global
SERVER scope or the EJB3 SUBSYSTEM scope.

At runtime all AOP has to do is:

MetaData metadata = MetaDataStack.peek();
AspectManager manager = metadata.getMetaData(AspectManager.class);

which will retrieve the scoped AOP domain if there is aop config at
the APPLICATION/SUBSYSTEM/etc.
or the default AspectManager at the SERVER level if there isn't.

[kabirkhan]

How do I turn on these new classloaders in jboss-head? I'd like to see how this all hangs together...

[kabirkhan]

Do you have some pointers to the classloading rules? I still need to get my head around this part, but in the meantime do you think we need special ClassPools as well?