8 Replies Latest reply on Feb 26, 2010 6:44 AM by flavia.rainone

    ScopedClassPoolRepositoryImpl and default ClassPool

    flavia.rainone

      While trying to fix the JBoss AOP + AS tests with the new jboss-classpool project, I bumped into a problem involving ScopedClassPoolRepositoryImpl and DefaultClassPool.

       

      Our factories were using DefaultClassPool to represent null class loaders so that bootstrap classes could be loaded without duplicates. This turned out to be a problem because we were seeing duplicates of JBoss normal classes. Take a look at this example:

      -> BaseClassLoader vfsfile:xxx/server/all/conf/jboss-service.xml belongs to DefaultDomain.

          When this ClassLoader is requested to load class org.jboss.jms.client.container.StateCreationAspect, it  will delegate to DefaultDomain, which will in turn find the original BaseClassLoader as a candidate to load that class. That BaseClassLoader loads the class and the result is returned.

       

      When this example is ported to ClassPool-level, we get a CNFException. The reason for this is that the BaseClassPool vfsfile:xxx/server/all/conf/jboss-service.xml has a parent that can find the resource that contains the requested class:


      public class TranslatableClassLoaderIsLocalResourcePlugin extends AbstractIsLocalResourcePlugin
      {
         public boolean isMyResource(String resourceName)
         {
            ClassLoader loader = getPool().getClassLoader();
            if (loader instanceof Translatable == false)
            {
               throw new IllegalStateException("ClassLoader of pool " + getPool() +  " is not instance of Translatable " + loader);
            }
            URL url = ((Translatable)getPool().getClassLoader()).getResourceLocally(resourceName);
            if (url == null)
            {
               return false;
            }
      
      RETURNS TRUE ----->      if (isSameInParent(resourceName, url))
            {
               return false;
            }
            return true;
         }
      
      }
      

       

      This is isSameInParent implementation:

       

      protected boolean isSameInParent(String classResourceName, URL foundURL)
         {
            ClassPool  parent = pool.getParent(); 
            if (parent != null)
            {
               ClassLoader parentLoader = parent.getClassLoader();
               URL parentURL = parentLoader.getResource(classResourceName);
               if (parentURL == null)
               {
                  return false;
               }
               URI parentURI = URI.create(parentURL.toString());
               URI foundURI = URI.create(foundURL.toString());
               if (parentURI.equals(foundURI))
               {
                  return true;
               }
            }
            
            return false;
         }

       

      And ClassPool.getClassLoader implementation:

       

      public ClassLoader getClassLoader()
      {
         return getContextClassLoader();
      }
      
      


      In the given example, getClassLoader will return the BaseClassLoader for AOP module, which can of course find the requested resource, even though it shouldn't.


      Another problem I had is that ScopedClassPoolRepositoryImpl edits the classpath of the DefaultClassPool:

      private ScopedClassPoolRepositoryImpl {
         classpool = ClassPool.getDefault();
         // FIXME This doesn't look correct
         ClassLoader cl = Thread.currentThread().getContextClassLoader();
         classpool.insertClassPath(new LoaderClassPath(cl));
      }
      

       

      This is similar to the problem explained above, and makes this class pool capable of loading classes that other classpools should be responsible to load. As a result, we get duplicates of the same CtClass. Its FIXME tells me that this was already spotted before as a possible bug.


      I worked around this issue by replacing the usage of the Default ClassPool by this Singleton class:

      public class SystemClassPool extends ClassPool
      {
         private SystemClassPool()
         {
            super(null);
            appendSystemPath();
         }
         
         @Override
         public ClassLoader getClassLoader()
         {
            return ClassLoader.getSystemClassLoader();
         }
      }
      

       

      Equally to DefaultClassPool, it has the SystemPath appended, but its getClassLoader method returns the SystemClassLoader, which is the closest I can get to Bootstrap class loader (take a look at the Javadoc for Class<?>. getResource for more on this).

       

      That didn't solve my problem entirely, because ScopedClassPoolRepositoryImpl has its own “default” classpool field:

      public class ScopedClassPoolRepositoryImpl implements ScopedClassPoolRepository {
        
          /** The default class pool */
          protected ClassPool classpool;
      
          /** The factory for creating class pools */
          protected ScopedClassPoolFactory factory = new ScopedClassPoolFactoryImpl();
      
          /**
           * Get the instance.
           * 
           * @return the instance.
           */
          public static ScopedClassPoolRepository getInstance() {
              return instance;
          }
      
          /**
           * Singleton.
           */
          private ScopedClassPoolRepositoryImpl() {
              classpool = ClassPool.getDefault();
              // FIXME This doesn't look correct
              ClassLoader cl = Thread.currentThread().getContextClassLoader();
              classpool.insertClassPath(new LoaderClassPath(cl));
          }
          ...
      }
      

       

      Differently from what one may have thought, the ClassPoolRepository class (from jboss-classpool project) is not a subclass of ScopedClassPoolRepositoryImpl, given this class cannot be subclassed. This has been an issue for me before, because I always thought my code would be much cleaner if I could extend ScopedClassPoolRepositoryImpl instead of delegating to it.

      So, what should be done in this regard? I'm opening this thread se we can find an elegant solution to my problem. Should ScopedClassPoolRepository be singleton? Should we use ScopedClassPoolFactory at all (the problem with not doing that is that we will have duplicate code in both classes)? I know that ScopedClassPool and auxiliary classes have been created to be used as the ClassPool solution to AS, but so many things have changed ever since that this solution is far from being complete, and that's why we need jboss-classpool now. So now, I'm not even sure what to do of that.


      Regarding ClassPool.getClassLoader()'s implementation, I don't think it is a bug, even though we are not using this implementation at all (ScopedClassPool overwrites it, and so does the new SystemClassPool class). I just think that it is a way of doing things that works with simple standalone scenarios, but that doesnot fulfill our needs with AS.

        • 1. Re: ScopedClassPoolRepositoryImpl and default ClassPool
          flavia.rainone

          Sorry about the font size, but it seems that I had a problem with the forum.

          flavia.rainone@jboss.com wrote:

           

          And ClassPool.getClassLoader implementation:

           

          public ClassLoader getClassLoader()
          {
             return getContextClassLoader();
          }
           
          

          Regarding ClassPool.getClassLoader()'s implementation, I don't think it is a bug, even though we are not using this implementation at all (ScopedClassPool overwrites it, and so does the new SystemClassPool class). I just think that it is a way of doing things that works with simple standalone scenarios, even though that doesnot fulfill our needs with AS.

          • 2. Re: ScopedClassPoolRepositoryImpl and default ClassPool
            flavia.rainone

            On a side note, the class pool that is used by factories to represent null class loaders is configured on org.jboss.classpool.spi.AbstractClassPoolFActory. Currently it can be set by callling AbstractClassPoolFactory.setDefaultClassPool() and it can be retrieved by callling AbstractClassPoolFactory.getDefaultClassPool().


            The problem is that, IMO, this nomenclature is not appropriate. Javassist uses default ClassPool to denote the classpools that is used by default on some operations. In my case, I'm configuring the class pool that is used to represent null class loaders.


            Any ideas on how to name those methods? I thought on setNullClassPool or on setBootstrapClassPool
            Notice that anyway we will have a gap with the SystemClassPool nomenclature, which is being used because this class pool is associated to the SystemClassLoader.

            • 3. Re: ScopedClassPoolRepositoryImpl and default ClassPool
              flavia.rainone

              On a private talk with Kabir and Ales, we decided that the best thing to do for now is to copy those ScopedCP classes to a new module of jboss-classpool project and "fix" everything that is getting in the way with those.

              As Kabir mentioned, those ScopedCP classes were created by us to be used inside AS. Now, they are outdated since they won't work in AS and we ended up needing ve a project only for that.


              I'm calling that new module scoped-classpool. Once it is ready, we can decide whether it would be a good idea to use those changes as a patch for Javassist, or whether we wanna keep it the way it is.

               

              I have only one observation to do regarding this. It will add a dependency from jboss-reflect towards scoped-classpool module. The way we are today, we have all references to the ScopedCP classes, and no reference at all to the subclasses in jboss-classpool project. Plus, we will also have to edit several references to ScopedCP's in JBoss AOP project as well.

              • 4. Re: ScopedClassPoolRepositoryImpl and default ClassPool
                flavia.rainone
                I created a Jira for this: JREFLECT-96
                • 5. Re: ScopedClassPoolRepositoryImpl and default ClassPool
                  flavia.rainone

                  flavia.rainone@jboss.com wrote:


                  I'm calling that new module scoped-classpool. Once it is ready, we can decide whether it would be a good idea to use those changes as a patch for Javassist, or whether we wanna keep it the way it is.

                  That new module is created. I removed everything that I thought is no longer being used, and replaced all references to javassist.scopedpool classes by references to the new org.jboss.classpool.scoped classes.

                  A summary of what has been done can be obtained by looking at the svn history log:

                  - Copied Scoped ClassPool classes from Javassist to a new scopedpool module.

                  - ScopedCPRepository is no longer singleton, it is an abstract class with a protected constructor instead. The default class pool can be defined as a parameter of the constructor.

                  - Replaced all references to javassist.scopedpool package by org.jboss.classpool.scoped package.
                  - Deleted ScopedClassPoolFactoryImpl class

                  Now I'm open for suggestions on what should be the next step: to write a patch for Javassist; to do an alpha release of the new version and update depending projects (JBoss AOP and JBoss Reflection, besides deployers-vfs tests); or to do more refactoring and improve something that doesn't look ok.

                  • 6. Re: ScopedClassPoolRepositoryImpl and default ClassPool
                    kabirkhan

                    flavia.rainone@jboss.com wrote:

                     

                    Now I'm open for suggestions on what should be the next step: to write a patch for Javassist; to do an alpha release of the new version and update depending projects (JBoss AOP and JBoss Reflection, besides deployers-vfs tests); or to do more refactoring and improve something that doesn't look ok.

                    I don't think we need to push this to javassist at this stage if that is what you mean.

                    • 7. Re: ScopedClassPoolRepositoryImpl and default ClassPool
                      alesj

                      Now I'm open for suggestions on what should be the next step: to write a patch for Javassist; to do an alpha release of the new version and update depending projects (JBoss AOP and JBoss Reflection, besides deployers-vfs tests); or to do more refactoring and improve something that doesn't look ok.

                      Whatever gets you back working on the Reflect+Javassist impl asap. ;-)


                      Check if there are no regressions with dependening projects and perhaps create a new Alpha release.

                       

                      Like I said, we should try to get Reflect fully working on Javassist + Classpools asap,

                      to actually see the whole picture: issues, benefits, bugz, optimizations, ...

                      In order to speed up things, Kabir is gonna help you with this -- you just tell him where you need him the most.

                       

                      Our target should be AS6_M3, it doesn't mean we need to be at 100%,

                      but we should be able to run a portion of testsuite with Reflect's TypeInfoFactory being Javassist based.

                      • 8. Re: ScopedClassPoolRepositoryImpl and default ClassPool
                        flavia.rainone

                        alesj wrote:

                         


                        Now I'm open for suggestions on what should be the next step: to write a patch for Javassist; to do an alpha release of the new version and update depending projects (JBoss AOP and JBoss Reflection, besides deployers-vfs tests); or to do more refactoring and improve something that doesn't look ok.

                        Whatever gets you back working on the Reflect+Javassist impl asap. ;-)


                        Check if there are no regressions with dependening projects and perhaps create a new Alpha release.

                         

                        Like I said, we should try to get Reflect fully working on Javassist + Classpools asap,

                        to actually see the whole picture: issues, benefits, bugz, optimizations, ...

                        In order to speed up things, Kabir is gonna help you with this -- you just tell him where you need him the most.

                        Ok! I'm doing the alpha release and then I'm updating the depending projects with it.

                        After that, I'm going to port the changes to JBoss AOP trunk, as we agreed on before.

                        IMO, this porting and updating things is work for one person only. Kabir could help me by doing the implementation of the generics features that we are going to need. Without that, we won't be able to to see the whole picture, and that's the main piece that we are missing right now.