ClassPoolRepository vs JBossclDelegatingClassPoolRepository

[flavia.rainone]

Since we updated the classpool and AOP versions at JBoss AS, we've been seeing some warning messages showing that unregistering of ClassPool is not working correctly.

The reason for these failures is that AspectManager is using ClassPoolRepository to register ClassLoaders, whereas the unregistering of ClassLoaders, performed by RegisterModuleCallback, is using JBossClDelegatingClassPoolRepository. As JBossClDelegatingClassPoolRepository.registeredModules map lacks the the register info, unregisterClassLoader is throwing an exception:

   public void unregisterClassLoader(ClassLoader classLoader, Module module)
   {
      ScopedClassPool classPool = registeredModules.remove(module);
      if (classLoader == null)
      {
         if (classPool == null)
         {
            throw new IllegalStateException("Module " + module + " is not registered");
         }

So, to fix this, AspectManager must start using JBossClDelegatingClassPoolFactory instead of ClassPoolFactory:

   public ClassPool registerClassLoader(ClassLoader ucl)
   {
      return ClassPoolRepository.getInstance().registerClassLoader(ucl);
   }

But this cannot be hard coded because JBossClDelegatingClassPoolFactory provides support to modules and assumes that you are using jboss-cl, which is only true in AS 5/6 environment. When using JBoss AOP in standalone mode or in JBoss AS 4, we need to use ClassPoolRepository.

The way that the ClassPool architecture is today, I'll have to make this configurable on the AOP side (e.g., I'll probably add this to AspectManagerJDK5). Is this the best option? Or should we rethink ClassPool architecture, making this configurable on the ClassPool side?

[flavia.rainone]

IMO, the ideal would to be use always ClassPoolRepository and set only the factoy. This would be cleanest possible. But, since the registering of jboss-cl ClassLoaders requires an extra step, we ended up with a JBossClDelegatingClassPoolRepository.

My suggestion is to get rid of JBossClDelegatingClassPoolRepository and, instead, change the spi to something like:

public class ClassPoolRepository
{
    public void setProfile(Profile profile)
}

Whereas profile would tell you both the factory and provide an extra plugin class containing any extra steps that may be required for classloader registration.

Another option would be keeping ClassPoolRepository the way it is, getting rid of JBossClDelegatingClassPoolRepository, and having the ClassPoolFactory providing a plugin class containing the extra steps required for classloader registration.

[kabirkhan]

Why not just have a public static void setClassPoolRepository(ClassPoolRepository) similar to setClassLoaderScopingPolicy() and setClassPoolFactory() and initialize that from AspectManagerServiceDelegate?

[alesj]

Why not just have a public static void setClassPoolRepository(ClassPoolRepository) similar to setClassLoaderScopingPolicy() and setClassPoolFactory() and initialize that from AspectManagerServiceDelegate?

No, we should have as litle as possible of static code.

e.g. perhaps even get rid of existing static code

Since static code is quite "evil" in OSGi based envs, which is what we aim in the future ... ;-)

[alesj]

The way that the ClassPool architecture is today, I'll have to make this configurable on the AOP side (e.g., I'll probably add this to AspectManagerJDK5). Is this the best option? Or should we rethink ClassPool architecture, making this configurable on the ClassPool side?

This sounds too much of an impl detail to be left to external (non-Classpool) libs to handle.

Hence my suggestion is to make this a spi/configuration on Classpool side,

so users (other libs) don't have to think about it when using it -- they simply use what Classpool provides.

[flavia.rainone]

Ales Justin wrote:

This sounds too much of an impl detail to be left to external (non-Classpool) libs to handle.

Hence my suggestion is to make this a spi/configuration on Classpool side,

so users (other libs) don't have to think about it when using it -- they simply use what Classpool provides.

In that case, I assume that the best option is:

(...) keeping ClassPoolRepository the way it is, getting rid of JBossClDelegatingClassPoolRepository, and having the ClassPoolFactory providing a plugin class containing the extra steps required for classloader registration.

This way, from an external point of view, all you have to do is: always use ClassPoolRepository; inject your ClassPoolFactory into ClassPoolRepository.

I implemented a first version of this as part of issue CLASSPOOL-2, which added to the spi package:

- a new interface, ClassLoaderRegistryHandler, responsible for handling the register and unregister calls in ClassPoolRepository

- a ClassLoaderRegistryHandlerFactory interface, that can be implemented by ClassPoolFactories that require a non-default ClassLoaderRegistryHandlers

That way, ClassPoolRepository.setClassPoolFactory checks for whether the CPFactory implements ClassLoaderRegistryHandlerFactory. If it does, ClassPoolRepository uses the factory to create a new ClassLoaderRegistryHandler for itself.

Plus:

- JBossClDelegatingClassPoolRepository has been renamed to JBossClRegistryHandler, is no longer a public class, and implements ClassLoaderRegistryHandler

- JBossclDelegatingClassPoolFactory implements ClassLoaderRegistryHandlerFactory so it can provide JBossClRegistryHandler to ClassPoolRepository

Let me know what you think of this implementation and what do you think should be changed/improved.