1 2 3 4 5 6 Previous Next 80 Replies Latest reply on Dec 14, 2009 4:50 PM by alesj Go to original post
      • 60. Re: Testing Deployers with new Reflect + ClassPool
        flavia.rainone

         

        "alesj" wrote:
        "flavia.rainone@jboss.com" wrote:

        That error is fully expected as the classes belonging to the excluded package must be excluded from the classpath during test execution :-)

        I guess you now do this explicitly in maven?

        You should do it programatically,
        the same way we do it in our CL, Deployers, Weld, ... tests.


        I added to the deployers-vfs tests the same check that generated the failure you saw on the classpool tests:

        ClassLoaderSystem system = (ClassLoaderSystem) getBean("ClassLoaderSystem");
        ClassLoaderDomain domain = system.getDefaultDomain();
        try{
         Class<?> clazz = domain.loadClass(JsfBean.class.getName());
         Assert.fail("Should not have been able to load " + JsfBean.class.getName());
        } catch(Exception expected) {}


        And it failed. How can I make this work programatically?

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

           

          "flavia.rainone@jboss.com" wrote:

          ClassLoaderSystem system = (ClassLoaderSystem) getBean("ClassLoaderSystem");
          ClassLoaderDomain domain = system.getDefaultDomain();
          try{
           Class<?> clazz = domain.loadClass(JsfBean.class.getName());
           Assert.fail("Should not have been able to load " + JsfBean.class.getName());
          } catch(Exception expected) {}


          And it failed.

          Debuging is your friend .... cccc!

          BaseClassLoaderDomain.class
           public Class<?> loadClass(String name)
           {
           try
           {
           return loadClass(null, name, true);
           }
           catch (ClassNotFoundException e)
           {
           return null;
           }
           }
          

          Hence no failure. ;-)


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

             

            "alesj" wrote:
             public Class<?> loadClass(String name)
             {
             try
             {
             return loadClass(null, name, true);
             }
             catch (ClassNotFoundException e)
             {
             return null;
             }
             }


            Now I'm curious... why isn't ClassNotFoundException being thrown? The ClassLoader.loadClass() API is clear about CNFE being thrown if a class can't be found.

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

              From a MicrocontainerTestDelegate, I have created a parent policy for the default domain that excludes all classes from package excludes. Now, if you try to do a domain.loadClass(A.class.getName()), for example, it will return null as expected. So far so good.

              But this does not seem to be enough. All tests are failing because of TestScenario.cannotLoadClass(L loader, String className).
              This method asserts that loader cannot load className. It is failing because the default domain enters this block:

              BaseClassLoaderPolicy policy = classLoader.getPolicy();
              ClassLoader hack = policy.isJDKRequest(name);
              if (hack != null)
              {
               if (trace)
               log.trace(this + " trying to load " + name + " using hack " + hack);
               Class<?> result = Class.forName(name, false, hack);
               if (result != null)
               {
               if (trace)
               log.trace(this + " loaded from hack " + hack + " " + ClassLoaderUtils.classToString(result));
               globalClassCache.put(path, new ClassCacheItem(result));
               return result;
               }
              }

              So I thought: adding the class to the JDKChecker.getExcludes collection should do the trick. The requesting class returned by AbstractJDKChecker.getRequestingClass() is SanityTestScenario. Adding this class to the excludes collection also causes the tests to fail. Now, calls to classLoader.loadClass(String className), that should return a class loaded by a BaseClassLoader, are returning a class loaded by the AppClassLoader.

              Does anybody has any ideas of a possible workaround to this?

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

                 

                "flavia.rainone@jboss.com" wrote:

                Now I'm curious... why isn't ClassNotFoundException being thrown? The ClassLoader.loadClass() API is clear about CNFE being thrown if a class can't be found.

                We're not loading class from ClassLoader, but from our custom API - ClassLoaderDomain / Loader.
                Hence we can do whatever we want.

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

                   

                  "flavia.rainone@jboss.com" wrote:
                  that should return a class loaded by a BaseClassLoader, are returning a class loaded by the AppClassLoader.

                  This sounds like the issue we had before -- classes "leaking" into system cl,
                  where we should really only allow java.* packages through.

                  What's the "L loader" in TestScenario.cannotLoadClass(L loader, String className)?
                  Could be by-passing some rules?

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

                     

                    "alesj" wrote:
                    "flavia.rainone@jboss.com" wrote:
                    that should return a class loaded by a BaseClassLoader, are returning a class loaded by the AppClassLoader.

                    This sounds like the issue we had before -- classes "leaking" into system cl,
                    where we should really only allow java.* packages through.

                    The point is that the failures I mentioned are being seen on sanity test cases, i.e., with ClassLoaders, no ClassPool involved.

                    "alesj" wrote:
                    What's the "L loader" in TestScenario.cannotLoadClass(L loader, String className)?
                    Could be by-passing some rules?

                    The L loader is just a generics abstraction for ClassLoaders and ClassPools, which allow me to write a test without being concerned whether it is going to be run against a ClassLoader or a ClassPool. The same test code is run with ClassLoaders (SanityTestCase) and with ClassPools (ClassPoolTestCase), thus enforcing that both behave the same way.

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

                      I bet I'm missing something simple.

                      Ales, I committed the adaptations I did for JBREFLECT-80 (the ones that are causing the errors I mentioned). Those adaptations are commented out with a TODO JBREFLECT-80. That should make it easier if you want to do any investigation to find out what is wrong. Just uncoment those and you will see the errors I am seeing.

                      Notice that there is a TODO JBREFLECT-80 on the pom.xml:

                      <move file="${project.build.testOutputDirectory}/org/jboss/test/classpool/support/excluded"
                       todir="${project.build.directory}/test-excluded-classes/org/jboss/test/classpool/support"/>
                      <!-- TODO JBREFLECT-80 -->
                      <!-- <copy todir="${project.build.directory}/test-excluded-classes/org/jboss/test/classpool/support">
                       <fileset dir="${project.build.testOutputDirectory}/org/jboss/test/classpool/support/excluded"/>
                      </copy> -->


                      You need to replace the move task by the copy one, that is commented out.
                      I haven't added the jars to the test resources yet because first I want to have JBREFLECT-80 working. Adding the jars to test resources is next in my list :-).

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

                         

                        "flavia.rainone@jboss.com" wrote:
                        I bet I'm missing something simple.

                        Indeed, there were a couple of errors in my implementation.

                        Once those are fixed, the excludes mechanism works flawlessly. However, I found out that:
                        - UCL tests are broken by that. Unless there is some excludes mechanism for UCL, I'll have to split the project before committing JBREFLECT-80.
                        - a few of the ClassPool tests are broken. I'll have to mimic the excludes mechanism in the ClassPools in order to have these tests working.



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

                           

                          "flavia.rainone@jboss.com" wrote:

                          - a few of the ClassPool tests are broken. I'll have to mimic the excludes mechanism in the ClassPools in order to have these tests working.

                          If the CP works properly on top of CL, it should not need such exclusion.


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

                             

                            "alesj" wrote:
                            "flavia.rainone@jboss.com" wrote:

                            - a few of the ClassPool tests are broken. I'll have to mimic the excludes mechanism in the ClassPools in order to have these tests working.

                            If the CP works properly on top of CL, it should not need such exclusion.

                            I also thought so, but I was wrong.
                            The ClassPools have their own parent and domain delegation structure, that mimics the ClassLoader's parent and domain structure. So, when we ask for a ClassPool to load a class, it will use this delegation structure to find and load the class. At the point that the delegation reaches the ClassPool corresponding to the AppClassLoader (the parent of default domain), the classpool is able of loading classes in excluded package. IMO, the only way of avoiding this is aborting the load of the class if it is in the excluded package. That's the same as saying that I have to mimic the exclusion mechanism with the classpools.

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

                               

                              "Ales" wrote:
                              If the CP works properly on top of CL, it should not need such exclusion.

                              I see your point. If the CP mimics the ClassLoader perfectly, it shouldn't have this problem. The problem here is that the CP doesn't mimic it perfectly.
                              For every classloader that is not a RealClassLoader, the JBossClDelegatingClassPoolFactory replaces a null parent by a default classPool:
                              if (parent == null)
                              {
                               parent = ClassPool.getDefault();
                              }
                              return new NonDelegatingClassPool(cl, parent, repository, true);


                              For example, if we have a URLClassLoader as the parent of a domain, the JBossClDelegatingCPFactory will create a NonDelegatingclassPool using the piece of code above to represent the parent of the domain. And, since the URLClassLoader, in cases like this, has a null parent, the factory will set the default ClassPool as the parent of the NonDelegatingClassPool (the Default ClassPool is a classpool that contains the system classpath).

                              Because of such a non-null parent, the exclusion is not working for class pools. :-(

                              On the other hand, if we kept the parent classpool as null, and if this non delegating classpool is required to load, say, java.lang.String, it will return a CtClass created by the NonDelegatingClassPool itself. This results in duplicates. Another NonDelegatingClassPool willl create its own CtClass representing String.class and so on. I'm not sure what are the consequences of this.

                              Interestingly, this problem doesn't occur with primitives. Javassist has an array of CtPrimitiveTypes, a subclass of CtClass, representing each one of the primitive types. The same collection is used for all classpools and if you try to get the classpool that loaded the primitive type, you get a null classpool. That's what I would like to see with a CtClass representing a String, for example. But creating a CtClass array with all classes of the bootstrap classpath is cumbersome, to say the least.

                              This is what I'll do. I'll leave the parent as null in the case above, which will make the exclusion mechanism work. And I'll try to filter out classes belonging to java.* and com.sun.* packages. Every class name that matches those will have an universal CtClass created for it (following the same that is done for primitives in the ClassPool), associated with a null ClassPool. For obvious reasons, I'm not very fond of this approach, but I think it is worth a shot, since it is this or implementing exclusion for classpools. Plus, I think it is the closest we can get to the ClassLoader's behaviour.

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

                                 

                                "flavia.rainone@jboss.com" wrote:

                                This is what I'll do. I'll leave the parent as null in the case above, which will make the exclusion mechanism work. And I'll try to filter out classes belonging to java.* and com.sun.* packages. Every class name that matches those will have an universal CtClass created for it (following the same that is done for primitives in the ClassPool), associated with a null ClassPool. For obvious reasons, I'm not very fond of this approach, but I think it is worth a shot, since it is this or implementing exclusion for classpools. Plus, I think it is the closest we can get to the ClassLoader's behaviour.

                                OK, I see what you mean, and you have a point there. :-)

                                I wouldn't go with this null, but just with a simple CP exclude -- only for tests purposes.
                                e.g.
                                Map the AppCL with DefaultCP, but somehow exclude all but java.* and our test packages.
                                Actually make this exclusion configurable, as we have it in CL/Deployers.


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

                                   

                                  "alesj" wrote:
                                  I wouldn't go with this null, but just with a simple CP exclude -- only for tests purposes.
                                  e.g.
                                  Map the AppCL with DefaultCP, but somehow exclude all but java.* and our test packages.
                                  Actually make this exclusion configurable, as we have it in CL/Deployers.

                                  The point with null is to reproduce the fact that String.class.getClassLoader() is null.
                                  The problem is that we won't be able of mapping all the bootstrap just by matching the package names.
                                  If you prefer to have the exclusion for ClassPools, then we will go that way. I'll let you know when this is done so you can finally debug the failure you are seeing.

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

                                     

                                    "flavia.rainone@jboss.com" wrote:
                                    "alesj" wrote:
                                    I wouldn't go with this null, but just with a simple CP exclude -- only for tests purposes.
                                    e.g.
                                    Map the AppCL with DefaultCP, but somehow exclude all but java.* and our test packages.
                                    Actually make this exclusion configurable, as we have it in CL/Deployers.

                                    The point with null is to reproduce the fact that String.class.getClassLoader() is null.
                                    The problem is that we won't be able of mapping all the bootstrap just by matching the package names.
                                    If you prefer to have the exclusion for ClassPools, then we will go that way. I'll let you know when this is done so you can finally debug the failure you are seeing.


                                    I've done some investigation, trying to find a better solution, but it looks like there isn't one, really. The only thing I can do is doing this exclusion not invasive in the code, or as Ales said, making it configurable:
                                    interface ClassPoolParentFactory
                                    {
                                     public ClassPool getClassPoolParent(ClassLoader classLoader)
                                    }
                                    
                                    class DefaultClassPoolParentFactory implements ClassPoolParentFactory
                                    {
                                     public ClassPool getClassPoolParent(ClassLoader classLoader)
                                     {
                                     if (classLoader == null) return ClassPool.getDefault();
                                     else return repository.register(classLoader);
                                     }
                                    }
                                    


                                    And then, in the tests:
                                    class FilteredClassPoolParentFactory
                                    {
                                    
                                    
                                     public ClassPool getClassPoolParent(ClassLoader classLoader)
                                     {
                                     if (classLoader == null)
                                     return new FilteredClassPoolAdapter(ClassPool.getDefault(), classFilter);
                                     else return repository.register(classLoader);
                                     }
                                    }
                                    
                                    class FilteredClassPoolAdapter extends ClassPool
                                    {
                                     private ClassPool delegate;
                                     private ClassFilter filter;
                                    
                                     public void get(String className) throws NotFoundException
                                     {
                                     if (filter.matchesClassName(className))
                                     {
                                     delegate.get(className);
                                     }
                                     throw new NotFoundException("...");
                                     }
                                     ....
                                    }