1 2 3 Previous Next 80 Replies Latest reply on Dec 14, 2009 4:50 PM by alesj

    Testing Deployers with new Reflect + ClassPool

    alesj

      Reporting my findings on the new ClassPoolTestCase:
      - http://anonsvn.jboss.org/repos/jbossas/projects/jboss-deployers/trunk/deployers-vfs/src/test/java/org/jboss/test/deployers/vfs/classpool/test/ClassPoolTestCase.java

      All the simple deployment tests pass,
      it was the more hierarchy complex (if you can call .ear complex) that immediately failed for me.

      It appeared that the old ClassPool's ClassLoader was used:

      junit.framework.AssertionFailedError: expected:<BaseClassLoader@135da43{vfs://top-level.ear/}> but was:<BaseClassLoader@2cbc86{vfs://simple.jar/}>
       at org.jboss.test.deployers.vfs.classpool.test.ClassPoolTest.assertClassPool(ClassPoolTest.java:83)
       at org.jboss.test.deployers.vfs.classpool.test.ClassPoolTestCase.testBasicEar(ClassPoolTestCase.java:82)


      Then I tied the RegisterModuleCallback with ClassLoading to properly track Module creation/destruction.
       <bean name="RegisterModuleCallback" class="org.jboss.classpool.plugins.as5.RegisterModuleCallback">
       <install method="addModuleRegistry" bean="ClassLoading" whenRequired="Start">
       <parameter><this/></parameter>
       </install>
       <uninstall method="removeModuleRegistry" bean="ClassLoading" whenRequired="Start">
       <parameter><this/></parameter>
       </uninstall>
       </bean>
      


      This now looks like something simply snatches CL under the ClassPool:
      junit.framework.AssertionFailedError: expected:<BaseClassLoader@bafdff{vfs://top-level.ear/}> but was:<null>
      


      Also, the assertClassPool method doesn't look valid for all classes.
      e.g. top deployment unit's classloader cannot see war's classes --> AnyServlet.class in testBasicEar


        • 1. Re: Testing Deployers with new Reflect + ClassPool
          flavia.rainone

           

          "alesj" wrote:
          All the simple deployment tests pass,
          it was the more hierarchy complex (if you can call .ear complex) that immediately failed for me.

          That's strange... I did a clean checkout of jboss deployers after committing and the test passed for me.
          "alesj" wrote:
          This now looks like something simply snatches CL under the ClassPool:
          junit.framework.AssertionFailedError: expected:<BaseClassLoader@bafdff{vfs://top-level.ear/}> but was:<null>
          


          Also, the assertClassPool method doesn't look valid for all classes.
          e.g. top deployment unit's classloader cannot see war's classes --> AnyServlet.class in testBasicEar

          I'll take a look.

          • 2. Re: Testing Deployers with new Reflect + ClassPool
            alesj

             

            "flavia.rainone@jboss.com" wrote:
            "alesj" wrote:
            All the simple deployment tests pass,
            it was the more hierarchy complex (if you can call .ear complex) that immediately failed for me.

            That's strange... I did a clean checkout of jboss deployers after committing and the test passed for me.

            This shouldn't pass wrt current test methods impl.

            But I guess in this case, where we're testing CL,
            we should have WarCLDeployerMock, which changes things a bit, at least for .wars.

            Although I still don't see how you can pass it,
            if the old ref is never cleaned up - or how we track Module usage?


            • 3. Re: Testing Deployers with new Reflect + ClassPool
              flavia.rainone

              It will always be a mistery why the tests passed before (but I swear they did ;)

              Yes, the classpools are never cleaned up, and this is generating the failures. This is a basic thing that I completely missed when doing the ClassPools. When I tested those against Branch5_x and JBoss AOP, they worked, but the point is that JBoss AOP has the mechanisms to clean the ClassPools.

              So, this is how JBoss AOP regiesters and unregisters ClassLoaders:
              1. The AspectDeployer registers the ClassLoader when an aspect is deployed.
              2. The AspectManager translator is is attached to the ClassLoaderSystem, which invokes the unregisterClassLoader everytime a unit is undeployed.

              On our side of the story, the ClassLoader is registered on an as-needed basis, by RepositoryClassPoolFactory.getPoolForLoader(ClassLoader). I dunno it would be a good idea to create a translator just for the sake of unregistering classloaders. What would be a better approach? Maybe a LifecycleCallback?

              • 4. Re: Testing Deployers with new Reflect + ClassPool
                alesj

                 

                "flavia.rainone@jboss.com" wrote:

                On our side of the story, the ClassLoader is registered on an as-needed basis, by RepositoryClassPoolFactory.getPoolForLoader(ClassLoader). I dunno it would be a good idea to create a translator just for the sake of unregistering classloaders. What would be a better approach? Maybe a LifecycleCallback?

                Uf, translator idea is horrible. ;-)

                We are already able to track when the ClassLoader goes away:
                when the Module is discarded --> ClassLoading::removeModule.

                And this is what I already fixed, so that the RegisterModuleCallback is part of this ClassLoading::removeModule call.
                It just looks like this doesn't do all the things we need to properly cleanup ClassPools.

                Or perhaps we already cleanup - as that null would indicate,
                it's just that we don't properly re-set things; e.g. in 2nd call to the TypeInfo/ClassPool getClassLoader.


                • 5. Re: Testing Deployers with new Reflect + ClassPool
                  flavia.rainone

                   

                  "alesj" wrote:
                  We are already able to track when the ClassLoader goes away:
                  when the Module is discarded --> ClassLoading::removeModule.

                  And this is what I already fixed, so that the RegisterModuleCallback is part of this ClassLoading::removeModule call.
                  It just looks like this doesn't do all the things we need to properly cleanup ClassPools.


                  I'm not properly cleaning up ClassPools:

                  public synchronized void removeModule(Module module)
                   {
                   logger.debug("Removing module " + module);
                   registeredModules.remove(module);
                   unregisteredModules.remove(module);
                   }


                  I need to invoke ClassPoolRepository.getInstance().unregisterClassLoader(ClassLoader). But how do I get the Module's classloader if Module.getClassLoader() is protected?

                  • 6. Re: Testing Deployers with new Reflect + ClassPool
                    alesj

                     

                    "flavia.rainone@jboss.com" wrote:

                    But how do I get the Module's classloader if Module.getClassLoader() is protected?

                    ClassLoading::getClassLoaderForModule(Module).

                    • 7. Re: Testing Deployers with new Reflect + ClassPool
                      flavia.rainone

                       

                      "alesj" wrote:
                      ClassLoading::getClassLoaderForModule(Module).


                      That fixed the tests for me. Could you please check if they pass for you as well?

                      Just an observation: I haven't made a new snapshot of jboss-classpool, so you need to build that project first before running deployers-vfs tests.

                      • 8. Re: Testing Deployers with new Reflect + ClassPool
                        alesj

                         

                        "flavia.rainone@jboss.com" wrote:

                        That fixed the tests for me. Could you please check if they pass for you as well?

                        Yup, it passes now.

                        "flavia.rainone@jboss.com" wrote:

                        Just an observation: I haven't made a new snapshot of jboss-classpool, so you need to build that project first before running deployers-vfs tests.

                        I deployed a new snapshot.

                        Now - as discussed - you need to add proper .war CL deployer
                        + fix assertClassPool to understand this hierarchy cl notion.


                        • 9. Re: Testing Deployers with new Reflect + ClassPool
                          alesj

                          Just a simple note.
                          Currently we have a failing Deployers test,
                          due to ongoing Classpool work / fixes.

                           public void testHierarchyCLUsage() throws Exception
                           {
                           AssembledDirectory directory = createBasicEar();
                           DeploymentUnit unit = assertDeploy(directory);
                           try
                           {
                           TypeInfoFactory typeInfoFactory = new JavassistTypeInfoFactory();
                           DeploymentUnit child = getDeploymentUnit(unit, "simple.war");
                           ClassLoader cl = getClassLoader(child);
                           TypeInfo ti = typeInfoFactory.getTypeInfo(AnyServlet.class.getName(), cl);
                           ClassInfo ci = assertInstanceOf(ti, ClassInfo.class);
                           MethodInfo mi = ci.getDeclaredMethod("getBean");
                           assertNotNull("No such 'getBean' method on " + ci, mi);
                           TypeInfo rt = mi.getReturnType();
                           TypeInfo cti = typeInfoFactory.getTypeInfo(PlainJavaBean.class.getName(), getClassLoader(unit));
                           assertSame(rt, cti); // current failure!
                           }
                           finally
                           {
                           undeploy(unit);
                           }
                           }
                          


                          • 10. Re: Testing Deployers with new Reflect + ClassPool
                            flavia.rainone

                            The cause of the failure is that ClassPoolDomain was not copying the structure of a domain when the domain's parent is a ClassLoaderToLoader adapter.

                            I have committed a temporary ugly hack, but I need to figure out how can I retrieve the classLoader in this scenario:

                            if (domain.getParent() instanceof ClassLoaderToLoaderAdapter)
                            {
                             // how to get the domain.getParent()'s classloader???
                             ClassLoader loader = ...;
                             ClassPool parentPool = ClassPoolRepository.getInstance().registerClassLoader(loader);
                             poolDomain = new JBossClClassPoolDomain(domain.getName(), parentPool, domain.getParentPolicy(), registry);
                            }


                            • 11. Re: Testing Deployers with new Reflect + ClassPool
                              alesj

                               

                              "flavia.rainone@jboss.com" wrote:

                              I have committed a temporary ugly hack, but I need to figure out how can I retrieve the classLoader in this scenario.

                              I've now at least removed the super horrible hack,
                              but this code definitely needs a proper cleanup,
                              as I've still found a bunch of weird things; catching NPE to do some logic, ...

                              I've currently used reflection to get to the underlying classloader.
                              This might not be so hackish, but we could still make some effort to see if there is a better way of doing this.

                               static
                               {
                               getClassLoader = AccessController.doPrivileged(new PrivilegedAction<Method>()
                               {
                               public Method run()
                               {
                               try
                               {
                               Method method = BaseClassLoaderSource.class.getDeclaredMethod("getClassLoader");
                               method.setAccessible(true);
                               return method;
                               }
                               catch (NoSuchMethodException e)
                               {
                               throw new RuntimeException("Cannot get classloader from " + BaseClassLoaderSource.class.getName(), e);
                               }
                               }
                               });
                               }
                              
                               static ClassLoader getClassLoader(BaseClassLoaderSource clSource)
                               {
                               try
                               {
                               return (ClassLoader)getClassLoader.invoke(clSource);
                               }
                               catch (Exception e)
                               {
                               throw new RuntimeException(e);
                               }
                               }
                              


                              • 12. Re: Testing Deployers with new Reflect + ClassPool
                                alesj

                                With so many ifs in Classpool project, we definitely need more CL hierarchy CP/TypeInfo tests in Deployers.

                                * CL isolation check
                                * Domain hierarchy usage
                                * Import from another bundle, although class exists in our jar
                                * Do things still properly work after re-deploy; e.g. no stale stuff left over
                                * ...

                                This is all crucial before we even try to make Javassist as default Reflect impl.

                                • 13. Re: Testing Deployers with new Reflect + ClassPool
                                  flavia.rainone

                                   

                                  "alesj" wrote:
                                  I've now at least removed the super horrible hack,
                                  but this code definitely needs a proper cleanup,
                                  as I've still found a bunch of weird things; catching NPE to do some logic, ...

                                  Those things were also part of the super horrible hack and defintely will be removed.

                                  "alesj" wrote:
                                  I've currently used reflection to get to the underlying classloader.
                                  This might not be so hackish, but we could still make some effort to see if there is a better way of doing this.

                                  TBH, I don't like using reflection because of performance. Any other ideas?

                                  • 14. Re: Testing Deployers with new Reflect + ClassPool
                                    flavia.rainone

                                     

                                    "alesj" wrote:
                                    With so many ifs in Classpool project, we definitely need more CL hierarchy CP/TypeInfo tests in Deployers.

                                    * CL isolation check
                                    * Domain hierarchy usage
                                    * Import from another bundle, although class exists in our jar
                                    * Do things still properly work after re-deploy; e.g. no stale stuff left over
                                    * ...

                                    This is all crucial before we even try to make Javassist as default Reflect impl.


                                    I think that we should look into the class pool tests and see what they are missing before we proceed to deployers tests. I'll start adding a test to reproduce the failure I caught yesterday.

                                    1 2 3 Previous Next