Generated Classes not found if they do not match any of Base

[kabirkhan]

I don't think this is a problem for AOP since whenever we generate classes they will be using the package of one of the classes in the classloader used to generate the class, but I thought I'd point out my following finding.

I create a domain with 2 BaseClassLoaders. classLoaderA has package blah.a, and classLoaderB has package blah.b. Now if I use javassist to create a class called not.registered.package.SomeClasss in ClassLoaderA then

//works since the class is registered in the loader's loaded classes
classLoaderA.loadClass("not.registered.package.SomeClasss");
//Does not work since not.registered.package is not in the
//list of packages of any of the loaders registered in the domain
classLoaderA.loadClass("not.registered.package.SomeClasss");

[kabirkhan]

The previous example was wrong it should have been:

//works since the class is registered in the loader's loaded classes
classLoaderA.loadClass("not.registered.package.SomeClasss");

//Does not work since not.registered.package is not in the
//list of packages of any of the loaders registered in the domain
classLoaderB.loadClass("not.registered.package.SomeClasss");

[kabirkhan]

Actually, this is a problem for the AOP proxies, which may be in the default package when creating a proxy for something in packages starting with java. or sun.

Digging into this I found the following:

ClassLoader loaderA = //ClassLoader belonging to DefaultDomain with package a
ClassLoader loaderB = //ClassLoader belonging to DefaultDomain with package b

//These all work
Class<?> aA = loaderA.loadClass("a.A");
Class<?> bB = loaderB.loadClass("b.B");
Class<?> bA = loaderB.loadClass("a.A");
Class<?> aB = loaderA.loadClass("b.B");

//Define a new class x.X in loaderA
CtClass clazz = poolA.makeClass("x.X"); //poolA uses loaderA
Class<?> x = clazz.toClass(); //defined using loaderA
Class<?> a.X = clazz.loadClass("x.X"); //works
Class<?> bX = clazz.loadClass("x.X"); throws ClassNotFoundException

The call to toClass() ends up here in JBossClDelegatingClassPool

 public Class<?> toClass(ToClassInvokerPoolReference pool, CtClass cc, String classFileName, ClassLoader loader, ProtectionDomain domain) throws CannotCompileException
 {
 ...
 try
 {
 URL outputURL = new URL(tempURL.toString() + "/" + classFileName);
 //Write the classfile to the temporary url
 synchronized (tmplock)
 {
 if (trace) logger.trace(this + " " + pool + ".toClass() myloader:" + myloader + " writing bytes to " + tempURL);
 ByteArrayOutputStream byteout = new ByteArrayOutputStream();
 BufferedOutputStream out = new BufferedOutputStream(byteout);
 out.write(cc.toBytecode());
 out.flush();
 out.close();

 byte[] classBytes = byteout.toByteArray();
 MemoryContextFactory factory = MemoryContextFactory.getInstance();
 factory.putFile(outputURL, classBytes);

 if (myloader instanceof RealClassLoader)
 {
 ((RealClassLoader)myloader).clearBlackList(classFileName);
 }

 Class<?> clazz = myloader.loadClass(cc.getName());
 if (trace) logger.trace(this + " " + pool + " myloader:" + myloader + " created class:" + clazz);
 return clazz;
 }
 }
 catch(Exception e)
 {
 ClassFormatError cfe = new ClassFormatError("Failed to load dyn class: " + cc.getName() + " on " + this + " loader:" + myloader);
 cfe.initCause(e);
 throw cfe;
 }
 }

It adds the class bytes to the vfs memory context associated with loaderA.
Next it loads the class. Internally this hits the domain first, and x.X gets added to the domain's globalClassBlackList. When the class is not found in the domain, it tries the initiating loader (loaderA), which can load the class.

When we try to load the class using loaderB, it cannot find the class using loaderB since loaderB will hit the domain, and the class is in the globalClassBlackList. This should be fixable by making BaseClassLoader.clearBlackList also remove the entry from its domain's globalClassBlackList?

However, the next problem then becomes updating the domain's classLoadersByPackageName since it knows nothing about package "x". I will see if I can recreate this somehow with a test in jboss-cl, and propose a fix.

What I have mentioned so far is for the mode where everything is visble in the whole domain. There will probably be further problems with OSGi style loading since we then need to update the modules?

[kabirkhan]

I have added a test showing this behaviour
http://fisheye.jboss.com/changelog/JBossAS/projects/jboss-cl/trunk?cs=83351

[kabirkhan]

The following patch fixes the problem:

$svn diff
Index: classloader/src/main/java/org/jboss/classloader/spi/base/BaseClassLoaderDomain.java
===================================================================
--- classloader/src/main/java/org/jboss/classloader/spi/base/BaseClassLoaderDomain.java (revision 83104)
+++ classloader/src/main/java/org/jboss/classloader/spi/base/BaseClassLoaderDomain.java (working copy)
@@ -358,7 +358,28 @@
 if (trace)
 log.trace(this + " trying to load " + name + " from requesting " + classLoader);
 if (classLoader.getResourceLocally(name) != null)
+ {
+ //The class might be generated in a new package, we need to update the information
+ clearBlackList(name);
+ if (allExports)
+ {
+ String packageName = ClassLoaderUtils.getResourcePackageName(name);
+ synchronized (classLoaders)
+ {
+ List<ClassLoaderInformation> list = classLoadersByPackageName.get(packageName);
+ if (list == null)
+ {
+ list = new CopyOnWriteArrayList<ClassLoaderInformation>();
+ classLoadersByPackageName.put(packageName, list);
+ }
+ if (list.contains(info) == false)
+ {
+ list.add(info);
+ }
+ }
+ }
 return classLoader.getLoader();
+ }
 }

It is a first effort, and I daren't commit it until somebody has taken a look :-)

I am unsure of whether this should be done, or how it would work, but I need to look into the possibility of doing something similar for when DelegateLoaders are used so that the information is reflected in Module.

[adrian.brock]

I don't get the point of this thread?

You can't load a class from package "x" unless the classloader declares
that it exports that package.

Your "fix" just looks like a receipe to bypass the import/export constraints by automatically
making visible unexported classes?

The ClassLoaderInformation is just part of the internal cache that remembers which
classloaders export which packages.

[kabirkhan]

I agree that classes should not get generated in another qualified package, so my test GeneratedClassesUnitTestCase. testImportAllGenerateClassInNewPackage() can be scrapped. AFAICT this will never happen in AOP, so it can go.

However, GeneratedClassesUnitTestCase.testImportAllGenerateClassInDefaultPackage() should remain. Say you are generating an AOP proxy for java.util.ArrayList() for a deployment. We cannot generate new classes in the java.util. package, so they are generated in the default package. The classloader used to create the proxy is the classloader of the deployment, since classes loaded by the bootstrap cannot see the ones from the aop (needed to access things like the Advisor, and the default interfaces the proxy gets such as AspectManaged) and deployment layers (in case of deployment specific inteface introductions).
https://jira.jboss.org/jira/browse/JBAS-5505

[adrian.brock]

I see your difficulty, but for the reasons stated above,
we can't just implicitly start exporting the default package
since that could reveal "module private" classes that the user doesn't want to be shared.

Wouldn't it make more sense to have these "java.xxx proxies" in some jboss aop
package that its classloader explicitly exports and define then against jboss aop's classloader?

That way your not going to interfere with the import/export rules of the user classloaders.

I guess the question Is what is the reason why you define these classes against
the user classloader instead of having one implementation against the jboss-aop
classloader or even the system classloader?

[adrian.brock]

I see your difficulty, but for the reasons stated above,
we can't just implicitly start exporting the default package
since that could reveal "module private" classes that the user doesn't want to be shared.

Wouldn't it make more sense to have these "java.xxx proxies" in some jboss aop
package that its classloader explicitly exports and define then against jboss aop's classloader?

I would have less qualms about an org.jboss.aop.xxx package automatically
exported by user classloaders since it is unlikely to be a user package
and it is trivial to warn/document that it happens if you use choose to use jboss aop.

[adrian.brock]

I guess the question Is what is the reason why you define these classes against
the user classloader instead of having one implementation against the jboss-aop
classloader or even the system classloader?

I'd further guess that it doesn't really matter (unless you had some name mangling scheme)
since in the old UCL mechanism the first classloader to have these classes would
always be used to load the classes.

In fact, that would potentially be a memory leak if that first classloader got redeployed
while other classloaders were sharing those classes.

[adrian.brock]

or even the system classloader?

The system classloader would probably be a bad idea if the aop classes themselves
got redeployed. ;-)

[kabirkhan]

I can generate the proxies in org.jboss.aop.generatedproxies package in the user's classloader.

Can you guys make the necessary changes to jboss-cl/AS deployers for this to work? Or let me know where to fix this.

[adrian.brock]

I can generate the proxies in org.jboss.aop.generatedproxies package in the user's classloader.

Can you guys make the necessary changes to jboss-cl/AS deployers for this to work? Or let me know where to fix this.

Hold your horses. :-)

You haven't answered why it is defined against the user classloader?
See my point about classloader leaks.

Having every classloader export that package will make the classloader
search in that package slow. It will need to check every classloader.
Of course once it has done it once, the result will be cached
(at least until somebody deploys/undeploys a classloader).

We also haven't discussed your other point about needing to flush the blacklist.

START OF IMPLEMENTATION DISCUSSION

Anyway, there's a number of ways to do it. It really depends upon how much you
want to make this configurable, i.e. allow different configuration per classloader ,etc.
and how you want to use it.

The simplest mechanism would be to add an "always exported packages"
to the ClassLoading bean. Then in Module.getCapabilities() you add
those package capabilities to what the modules decides by the Module itself
or are generated by the "defaultCapabilities".

But this could be generalized to "global capabilities' rather than
"always exported packages" such that the Classloading bean could be configured
with packages that have a version if that is important?

e.g.

<bean name="ClassLoading">
 <property name="GlobalCapabilities">
 <capabilities xmlns="urn:jboss:classloading:1.0">
 <package name="org.jboss.aop.generatedclasses" version="2.0.0"/>
 </capabilities>
 </property>
</bean>

I'm not sure how useful this really is as a generalization? :-)

It also introduces complications:
1) if you somebody makes the mistake of adding a module capability that is global
then all modules would get the same module name as an alias :-)
2) Do we try to merge the global capabilities with what the classloader defines?
e.g. we ignore the global capability if the classloader defines version 2.0.1 of the same package?
etc.

[adrian.brock]

START OF IMPLEMENTATION DISCUSSION

Anyway, there's a number of ways to do it.

I'm sure Ales would want to do something more complicated
but more dynamic and pluggable such as defining a

public interface GlobalCapabilitiesProvider
{
 Capabilities getCapabilities()
}

The ClassLoading would then have an in/uncallback for GlobalCapabilitiesProvider
and one your aop beans would implement it or use a helper bean exposed by
the classloading module.

That way the configuration of your "always exported packages" is in the aop.xml
and only gets applied if aop.xml is deployed.

But others can also use this mechanism if it is required without having to
worry about mixing the configuration together in classloader.xml. :-)

[kabirkhan]

You haven't answered why it is defined against the user classloader?
See my point about classloader leaks.

It is defined against the user classloader since as far as I can tell:
-We cannot use the system classloader. The proxies use interfaces from the aop classloader (org.jboss.aop.proxy.container.AspectManaged and org.jboss.aop.proxy.container.Delegate) which are not visible from the system classloader.
-We cannot use the aop classloader. The proxies might have interfaces introduced by the deployment, whose classloader might be in a child domain of the aop classloader's domain. Those interfaces would not be visible from the aop classloader.

At the moment we are caching the generated proxies by deployment classloader. The classloader is the key into a WeakHashMap. If deploymentA wants to create a proxy for ArrayList with no interface introductions, we generate the proxy class using deploymentA's classloader. If deploymentB wants to create a similar proxy for ArrayList, we generate the proxy class using deploymentB's classloader. Subsequent attempts to get a plain ArrayList proxy from deploymentA's classloader will use deploymentA's cached class, and deploymentB will use deploymentB's cached class.

Doesn't your point about classloader leaks hold for any deployment?