1 2 Previous Next 24 Replies Latest reply on Dec 1, 2008 7:09 AM by alesj

    MC upgrade breaks scoped aop tests in trunk

    kabirkhan

      https://jira.jboss.org/jira/browse/JBAS-6255
      If an AspectDefinition and its aspect class is deployed in the default classloader domain, and a deployment in a child classloader domain with no parent delegation overrides that class, the child deployment should use its own copy of the class.

      Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory. However, now the underlying GenericBeanFactory will always use the loader of its KernelControllerContext to create the bean instance, so in the case outlined the wrong loader is used to create the bean.

      Rather than calling GBF directly from GBAF, if there is a pushed classloader I am trying to wrap GBF in a wrapper class that implements the same createBean() method, but with the option to specify the classloader. This gives me better results although I am still seeing some failures.

        • 1. Re: MC upgrade breaks scoped aop tests in trunk

           

          "kabir.khan@jboss.com" wrote:

          Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.


          How did this work? I don't see anything in the previous GBF code that allowed you
          to override the classloader (other than the metadata).

          However, now the underlying GenericBeanFactory will always use the loader of its KernelControllerContext to create the bean instance, so in the case outlined the wrong loader is used to create the bean.


          This hasn't changed. All that changed is that this retrieval is now in a privileged block.

          Asking the controller context essentially asks the ClassLoaderMetaData
          if it has one (otherwise it uses the TCL),
          which will be coming from whatever you setup in the deployer.

          AbstractAopMetaDataDeployer
           private void deploy(FakeComponentUnit unit, BeanMetaData deployment) throws DeploymentException
           {
           // No explicit classloader, use the deployment's classloader
           if (deployment.getClassLoader() == null)
           {
           try
           {
           // Check the unit has a classloader
           unit.getClassLoader();
           // TODO clone the metadata?
           deployment.setClassLoader(new DeploymentClassLoaderMetaData(unit)); // HERE
           }
           catch (Exception e)
           {
           log.debug("Unable to retrieve classloader for deployment: " + unit.getName() + " reason=" + e.toString());
           }
           }
          



          Rather than calling GBF directly from GBAF, if there is a pushed classloader I am trying to wrap GBF in a wrapper class that implements the same createBean() method, but with the option to specify the classloader. This gives me better results although I am still seeing some failures.


          The ClassLoaderMetaData is also used to decide the classloader for other things,
          e.g. string conversion to classes so I don't think being able to specify a classloader
          on the GBF method is much help without rewriting all the metadata classes to also
          allow the classloader to be overridden.

          • 2. Re: MC upgrade breaks scoped aop tests in trunk

             

            "adrian@jboss.org" wrote:
            "kabir.khan@jboss.com" wrote:

            Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.


            How did this work? I don't see anything in the previous GBF code that allowed you
            to override the classloader (other than the metadata).


            NOTE: Changing the ClassLoaderMetaData on the GenericBeanFactory runtime
            object won't help since that is just a copy of what the ControllerContext sees in
            the BeanMetaData.

            From GenericBeanFactoryMetaData
             if (classLoader != null)
             {
             builder.setClassLoader(classLoader); // Used by the ControllerContext
             builder.addPropertyMetaData("classLoader", builder.createValue(classLoader)); // Used by the GBF
             }
            


            • 3. Re: MC upgrade breaks scoped aop tests in trunk
              kabirkhan

               

              "adrian@jboss.org" wrote:
              "kabir.khan@jboss.com" wrote:

              Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.

              How did this work? I don't see anything in the previous GBF code that allowed you
              to override the classloader (other than the metadata).

              The "pushed" metadata was the mechanism. For some reason the GBF.getClassLoader() always returned null, and I think the KCC loader was ignored previously. Anyway, whatever was put in by the metadata did work.

              "adrian@jboss.org" wrote:

              Asking the controller context essentially asks the ClassLoaderMetaData
              if it has one (otherwise it uses the TCL),
              which will be coming from whatever you setup in the deployer.

              That is different from the ClassLoaderMetaData that is in GBF. Maybe not before I start messing around with things, but definitely after calling GBF.setClassLoader().

              "adrian@jboss.org" wrote:

              AbstractAopMetaDataDeployer
               private void deploy(FakeComponentUnit unit, BeanMetaData deployment) throws DeploymentException
               {
               // No explicit classloader, use the deployment's classloader
               if (deployment.getClassLoader() == null)
               {
               try
               {
               // Check the unit has a classloader
               unit.getClassLoader();
               // TODO clone the metadata?
               deployment.setClassLoader(new DeploymentClassLoaderMetaData(unit)); // HERE
               }
               catch (Exception e)
               {
               log.debug("Unable to retrieve classloader for deployment: " + unit.getName() + " reason=" + e.toString());
               }
               }
              


              Yes, that sets the correct classloader to use for the GenericBeanAspectFactory. But when needing to create an instance from a scoped classloader with no child delegation that also defines the class, that is the wrong loader (i.e. the parent). In all other cases this loader is fine.

              "adrian@jboss.org" wrote:


              Rather than calling GBF directly from GBAF, if there is a pushed classloader I am trying to wrap GBF in a wrapper class that implements the same createBean() method, but with the option to specify the classloader. This gives me better results although I am still seeing some failures.


              The ClassLoaderMetaData is also used to decide the classloader for other things,
              e.g. string conversion to classes so I don't think being able to specify a classloader
              on the GBF method is much help without rewriting all the metadata classes to also
              allow the classloader to be overridden.



              • 4. Re: MC upgrade breaks scoped aop tests in trunk
                kabirkhan

                What I am doing now and which makes all tests pass locally again is:

                GenericBeanAspectFactory:

                 protected Object doCreate(Advisor advisor, InstanceAdvisor instanceAdvisor, Joinpoint jp)
                 {
                 try
                 {
                 log.debug("Creating advice " + name);
                
                 Object object = null;
                 final ClassLoader loader = getCreatingClassLoader(advisor);
                 if (loader == null)
                 {
                 object = factory.createBean();
                 }
                 else
                 {
                 if (context == null)
                 {
                 throw new IllegalStateException("Attempting to create aspects in an undeployed factory");
                 }
                 KernelConfigurator configurator = context.getKernel().getConfigurator();
                 ClassLoaderAwareGenericBeanFactory factoryWrapper = new ClassLoaderAwareGenericBeanFactory(configurator, loader, (GenericBeanFactory)factory);
                 object = factoryWrapper.createBean();
                 }
                
                
                 if (object instanceof XmlLoadable)
                 {
                 ((XmlLoadable)object).importXml(element);
                 }
                 configureInstance(object, advisor, instanceAdvisor, jp);
                 return object;
                 }
                 catch (Throwable throwable)
                 {
                 throw new RuntimeException(throwable);
                 }
                 }
                
                 private ClassLoader getCreatingClassLoader(Advisor advisor)
                 {
                 if (advisor == null)
                 {
                 return getLoader();
                 }
                 return SecurityActions.getClassLoader(advisor.getClass());
                 }
                


                The createBean() and invokeLifecycle() methods are modified copies of the GBF and AbstractBeanFactory equivalents
                public class ClassLoaderAwareGenericBeanFactory implements BeanFactory
                {
                 ClassLoader loader;
                 GenericBeanFactory delegate;
                 KernelConfigurator configurator;
                
                 public ClassLoaderAwareGenericBeanFactory(KernelConfigurator configurator, ClassLoader loader, GenericBeanFactory factory)
                 {
                 this.configurator = configurator;
                 this.loader = loader;
                 this.delegate = factory;
                 }
                
                 public Object createBean() throws Throwable
                 {
                 if (loader == null)
                 {
                 return delegate.createBean();
                 }
                 else
                 {
                 BeanInfo info = null;
                 if (delegate.getBean() != null)
                 info = configurator.getBeanInfo(delegate.getBean(), loader, delegate.getAccessMode());
                
                 Joinpoint joinpoint = configurator.getConstructorJoinPoint(info, delegate.getConstructor(), null);
                 Object result = joinpoint.dispatch();
                 if (info == null && result != null)
                 info = configurator.getBeanInfo(result.getClass(),delegate.getAccessMode());
                
                 if (delegate.getProperties() != null && delegate.getProperties().size() > 0)
                 {
                 for (Map.Entry<String, ValueMetaData> entry : delegate.getProperties().entrySet())
                 {
                 String property = entry.getKey();
                 ValueMetaData vmd = entry.getValue();
                 PropertyInfo pi = info.getProperty(property);
                 pi.set(result, vmd.getValue(pi.getType(), loader));
                 }
                 }
                 invokeLifecycle("create", delegate.getCreate(), info, loader, result);
                 invokeLifecycle("start", delegate.getStart(), info, loader, result);
                 return result;
                
                 }
                 }
                
                 protected void invokeLifecycle(String methodName, LifecycleMetaData lifecycle, BeanInfo info, ClassLoader cl, Object target) throws Throwable
                 {
                 if (lifecycle == null || lifecycle.isIgnored() == false)
                 {
                 String method = methodName;
                 if (lifecycle != null && lifecycle.getMethodName() != null)
                 method = lifecycle.getMethodName();
                 List<ParameterMetaData> parameters = null;
                 if (lifecycle != null)
                 parameters = lifecycle.getParameters();
                 MethodJoinpoint joinpoint;
                 try
                 {
                 joinpoint = configurator.getMethodJoinPoint(info, cl, method, parameters, false, true);
                 }
                 catch (JoinpointException ignored)
                 {
                 return;
                 }
                 joinpoint.setTarget(target);
                 joinpoint.dispatch();
                 }
                 }
                
                }
                
                


                • 5. Re: MC upgrade breaks scoped aop tests in trunk

                   

                  "kabir.khan@jboss.com" wrote:
                  "adrian@jboss.org" wrote:
                  "kabir.khan@jboss.com" wrote:

                  Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.

                  How did this work? I don't see anything in the previous GBF code that allowed you
                  to override the classloader (other than the metadata).

                  The "pushed" metadata was the mechanism. For some reason the GBF.getClassLoader() always returned null,


                  I think this is the real change isn't it? It would depend upon how you
                  created/deployed the GBF's metadata. How old is the previous code I showed?


                  and I think the KCC loader was ignored previously. Anyway, whatever was put in by the metadata did work.


                  This is the code from 7 months ago before recent changes.
                  You can see it looks at the context first
                  http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas/projects/microcontainer/trunk/kernel/src/main/java/org/jboss/beans/metadata/plugins/factory/GenericBeanFactory.java?revision=77665&view=markup

                  But actually looking at the logic behind all this, I don't see why we need to look
                  at the context for classloader since the next step is to ask the
                  ClassLoaderMetaData anyway (or use the TCL absent that).

                   private Object createBean(ClassLoader cl) throws Throwable
                   {
                   ClassLoader loader = cl;
                   if (loader == null)
                   loader = Configurator.getClassLoader(classLoader);
                  


                  For everybody else, this will be same as asking the controller context
                  since the "classLoader" above is the same ClassLoaderMetaData.
                  But for you who wants to change the logic, it will do something different.

                  • 6. Re: MC upgrade breaks scoped aop tests in trunk

                     

                    "kabir.khan@jboss.com" wrote:

                    The createBean() and invokeLifecycle() methods are modified copies of the GBF and AbstractBeanFactory equivalents


                    Duplicating code just means it won't get fixed the next time somebody finds a bug
                    or adds a new feature.

                    • 7. Re: MC upgrade breaks scoped aop tests in trunk
                      kabirkhan

                       

                      "adrian@jboss.org" wrote:

                      I think this is the real change isn't it? It would depend upon how you
                      created/deployed the GBF's metadata. How old is the previous code I showed?


                      The AbstractAopMetaDataDeployer code is current trunk and has been there for some months
                      "adrian@jboss.org" wrote:

                      This is the code from 7 months ago before recent changes.
                      You can see it looks at the context first
                      http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas/projects/microcontainer/trunk/kernel/src/main/java/org/jboss/beans/metadata/plugins/factory/GenericBeanFactory.java?revision=77665&view=markup

                      Actually, GBF.classLoader is what is used if present. Not the context:
                       public Object createBean() throws Throwable
                       {
                       ClassLoader cl = null;
                       if (classLoader == null && context != null)
                       {
                       try
                       {
                       cl = context.getClassLoader();
                       }
                       catch (Throwable t)
                       {
                       log.trace("Unable to retrieve classloader from " + context);
                       }
                       }
                      
                       if (cl == null)
                       cl = Configurator.getClassLoader(classLoader);
                      


                      "adrian@jboss.org" wrote:

                      But actually looking at the logic behind all this, I don't see why we need to look
                      at the context for classloader since the next step is to ask the
                      ClassLoaderMetaData anyway (or use the TCL absent that).

                       private Object createBean(ClassLoader cl) throws Throwable
                       {
                       ClassLoader loader = cl;
                       if (loader == null)
                       loader = Configurator.getClassLoader(classLoader);
                      


                      For everybody else, this will be same as asking the controller context
                      since the "classLoader" above is the same ClassLoaderMetaData.
                      But for you who wants to change the logic, it will do something different.

                      So what do you suggest? Putting things back to more like the way they were? I've got an idea for a test that I can add to aop-mc-int that can help test for future regressions once this is working again.

                      "adrian@jboss.org" wrote:

                      Duplicating code just means it won't get fixed the next time somebody finds a bug
                      or adds a new feature.

                      True, I was just trying to find a way out of my Friday surprise mess :-)

                      • 8. Re: MC upgrade breaks scoped aop tests in trunk
                        kabirkhan

                        I have added a test to mc Branch_2_0 org.jboss.test.microcontainer.test.OverriddenClassLoaderTestCase. This will fail until this is fixed, but should help against regressions in this area in future.

                        • 9. Re: MC upgrade breaks scoped aop tests in trunk
                          dimitris

                          So who's fixing this / when? A new release of MC is needed?

                          • 10. Re: MC upgrade breaks scoped aop tests in trunk
                            alesj

                             

                            "dimitris@jboss.org" wrote:
                            So who's fixing this / when? A new release of MC is needed?

                            I can do the release (if needed), but dunno who's fixing this.

                            • 11. Re: MC upgrade breaks scoped aop tests in trunk
                              anil.saldhana

                               

                              "alesj" wrote:
                              "dimitris@jboss.org" wrote:
                              So who's fixing this / when? A new release of MC is needed?

                              I can do the release (if needed), but dunno who's fixing this.


                              While you are at the mc release, can you put back the system property for the real urls in VFSClassLoaderPolicy?

                              • 12. Re: MC upgrade breaks scoped aop tests in trunk
                                alesj

                                 

                                "anil.saldhana@jboss.com" wrote:
                                While you are at the mc release, can you put back the system property for the real urls in VFSClassLoaderPolicy?

                                MC != CL ;-)

                                Dimitris means Kernel part of MC.
                                We should re-name the MC parts, as MC of MC is confusing.

                                • 13. Re: MC upgrade breaks scoped aop tests in trunk
                                  kabirkhan

                                   

                                  "alesj" wrote:
                                  "dimitris@jboss.org" wrote:
                                  So who's fixing this / when? A new release of MC is needed?

                                  I can do the release (if needed), but dunno who's fixing this.


                                  I think you or Adrian need to do it, I'm not 100% clear on what Adrian means, but I think GBF needs to use the loaders in the same way as before?

                                  • 14. Re: MC upgrade breaks scoped aop tests in trunk
                                    alesj

                                     

                                    "kabir.khan@jboss.com" wrote:

                                    I think you or Adrian need to do it

                                    Why us?
                                    It's your feature. ;-)

                                    "kabir.khan@jboss.com" wrote:

                                    I'm not 100% clear on what Adrian means

                                    As much as I could follow, he's just trying to say you need
                                    to prepare metadata properly and things should work.
                                    Easy. :-)

                                    1 2 Previous Next