8 Replies Latest reply on Jan 6, 2010 1:41 PM by kabirkhan

    Optimizing aop lifecycle

    kabirkhan

      My original plan was to just hard-code the annotation plugins on startup, but I then realised that we probably still want to be able to deploy them. So, when this is parsed:

        <lifecycle-configure
                    name="LifecycleCallback"
                    class="org.jboss.test.microcontainer.support.LifecycleCallbackWithBeanDependency"
                    classes="@org.jboss.test.microcontainer.support.Lifecycle">
        </lifecycle-configure>
      
      

      A bean called LifecycleCallback implemented by LCWDB is installed (as before), and an instance of LifecycleAnnotationPlugin handling @Lifecycle is added by the LifecycleBinding bean.

       

      Previously instead of LAPlugin the LifecycleBinding bean installed a lifecycle pointcut to the AspectManager, and pointcut matching was done as part of AOPDependencyBuilder using the info from the AspectManager. AOPDepBuilder would create a LifecycleAspectDependencyBuilderListItem which would set up the correct dependencies and set up the lifecycle callback items in the matching context's dependency info. Lifecycle calbacks would then be invoked by AbstractController.handleLifecycleCallbacks().

       

      Now instead of the AOPDepBuilder doing fancy and heavy pointcut matching to determine lifecycle callbacks, the LifecycleAnnotationPlugin is invoked as part of DescibeAction.applyAnnotations() -> BeanAnnotationAdapter.applyAnnotations().

       

      If we deploy a bean that has the @Lifecycle annotation BAA picks that up and it is handled by LifecycleAnnotationAdapter which uses LifecycleAspectDependencyBuilderListItem to set up the dependencies and dependency info as before. I just used LADBLI since the code was already there, but am planning to refactor that somehow

       

      Anyway, the question I really had was: am I ok to expose the add/removeAnnotationPlugin() methods in the BeanAnnotationAdapter interface?

       

      Code from LifecycleBinding:

       

         public void start() throws Exception
         {
            .....
            if (!classes.startsWith("@"))
               throw new IllegalArgumentException("Could not parse '" + classes + " into an annotation. (It must start with '@')");
            String annotationName = classes.substring(1);
            Class<Annotation> clazz = null;
            try
            {
               clazz = (Class<Annotation>)SecurityActions.getContextClassLoader().loadClass(annotationName);
            }
            catch (Exception e)
            {
               throw new IllegalArgumentException("An error occurred loading '" + classes + "'", e);
            }
      
            LifecycleAspectDependencyBuilderListItem item = new LifecycleAspectDependencyBuilderListItem(callbackBean, state, installMethod, uninstallMethod);
            plugin = new LifecycleAnnotationPlugin<Annotation>(clazz, item);
            getBeanAnnotationAdapter().addAnnotationPlugin(plugin);
         }
      
      
         public void stop() throws Exception
         {
            if (plugin != null)
               getBeanAnnotationAdapter().removeAnnotationPlugin(plugin);
         }
      
         @SuppressWarnings("unchecked")
         private CommonAnnotationAdapter<AnnotationPlugin<?, ?>, MetaDataVisitor> getBeanAnnotationAdapter()
         {
            BeanAnnotationAdapter adapter = BeanAnnotationAdapterFactory.getInstance().getBeanAnnotationAdapter();
            if (adapter instanceof CommonAnnotationAdapter == false)
               throw new IllegalArgumentException("Adapter is not an instance of CommonAnnotationAdapter");
            return (CommonAnnotationAdapter<AnnotationPlugin<?,?>, MetaDataVisitor>)adapter;
         }
       
      

       

       

      public class LifecycleAnnotationPlugin<C extends Annotation> extends ClassAnnotationPlugin<C> implements CleanablePlugin
      {
         Logger log = Logger.getLogger(LifecycleAnnotationPlugin.class);
      
         private final DependencyBuilderListItem item;
      
         private final Set<KernelControllerContext> handledContexts = new ConcurrentSet<KernelControllerContext>();
      
         public LifecycleAnnotationPlugin(Class<C> annotation, DependencyBuilderListItem item)
         {
            super(annotation);
            if (item == null)
               throw new IllegalArgumentException("Null dependency builder list item");
            this.item = item;
         }
      
         @Override
         protected List<? extends MetaDataVisitorNode> internalApplyAnnotation(ClassInfo info, MetaData retrieval, C annotation, KernelControllerContext context) throws Throwable
         {
            item.addDependency(context);
            handledContexts.add(context);
            return null;
         }
      
         @Override
         protected void internalCleanAnnotation(ClassInfo info, MetaData retrieval, C annotation, KernelControllerContext context) throws Throwable
         {
            item.removeDependency(context);
            handledContexts.remove(context);
      
         }
      
         public void clean()
         {
            for (KernelControllerContext context : handledContexts)
            {
               try
               {
                  internalCleanAnnotation(null, null, null, context);
               }
               catch(Throwable t)
               {
                  log.warn("An error occurred cleaning " + context, t);
               }
            }
         }
      }
       
      

       

      The clean() method is called by CommonBeanAnnotationAdapter.removeAnnotationPlugin() to make sure that we remove the dependency from the contexts affected by the annotation.

        • 1. Re: Optimizing aop lifecycle
          kabirkhan
          Ignore all that cleanup stuff for the plugin, overriding isCleanup() does the job
          • 2. Re: Optimizing aop lifecycle
            kabirkhan

            So far the results are good, I am having a problem with UnwindLifeCycleTestCase though:

             

               <aop:lifecycle-install xmlns:aop="urn:jboss:aop-beans:1.0"
                           name="InstallAdvice1"
                           class="org.jboss.test.microcontainer.support.InstallUninstallLifecycleCallback"
                           classes="@org.jboss.test.microcontainer.support.Install">
               </aop:lifecycle-install>
            
               <aop:lifecycle-install xmlns:aop="urn:jboss:aop-beans:1.0"
                           name="InstallAdvice2"
                           class="org.jboss.test.microcontainer.support.InstallUninstallLifecycleCallback"
                           classes="@org.jboss.test.microcontainer.support.Install">
               </aop:lifecycle-install>
            
               <aop:lifecycle-install xmlns:aop="urn:jboss:aop-beans:1.0"
                           name="ErrorAdvice"
                           class="org.jboss.test.microcontainer.support.ErrorLifecycleCallback"
                           classes="@org.jboss.test.microcontainer.support.Error">
               </aop:lifecycle-install>
            
               <aop:lifecycle-install xmlns:aop="urn:jboss:aop-beans:1.0"
                           name="InstallAdviceNotInvoked"
                           class="org.jboss.test.microcontainer.support.InstallUninstallLifecycleCallback"
                           classes="@org.jboss.test.microcontainer.support.Install">
               </aop:lifecycle-install>
            
               <bean name="Bean" class="org.jboss.test.microcontainer.support.SimpleBeanImpl">
                  <annotation>@org.jboss.test.microcontainer.support.Install</annotation>
                  <annotation>@org.jboss.test.microcontainer.support.Error</annotation>
               </bean>
            
            Previously the callbacks would be triggered in the order they were defined, so that InstallAdvice1 would be invoked before InstallAdvice2 which would be invoked before ErrorAdvice. ErrorAdvice would throw an error, so that InstallAdviceNotInvoked would not be called.
            However, now the callbacks are invoked in the order the annotations are retrieved from MDR. So, if @Error is retrieved before @Install none of the @Install advices are called since ErrorAdvice throws an exception. Likewise, if @Install is retrieved before @Error, all the @Install advices are called.
            I am not sure if this is a problem or not? If not I can fix the test. In the real world the annotations will probably trigger callbacks doing unrelated things?
            The plugins per annotation in CommonAnnotationAdapter were kept in a Set, I have replaced it locally with a List so that at least for a given annotation the order is guaranteed, so for @Install InstallAdvice1 comes before InstallAdvice2 etc.
            • 3. Re: Optimizing aop lifecycle
              kabirkhan

              I am fixing the test by changing the ErrorAdvice to handle the same annotation as the othe install advices

                 <aop:lifecycle-install xmlns:aop="urn:jboss:aop-beans:1.0"
                             name="ErrorAdvice"
                             class="org.jboss.test.microcontainer.support.ErrorLifecycleCallback"
                             classes="@org.jboss.test.microcontainer.support.Install">
                 </aop:lifecycle-install>

               

              For the record, here is the JIRA https://jira.jboss.org/jira/browse/JBKERNEL-75

              • 4. Re: Optimizing aop lifecycle
                kabirkhan

                I have committed this and all tests pass. There are some changes in that lifecycle-xxx now throws an error if the 'expr' attribute is used, and the 'classes' attribute must now be an annotation.

                 

                From LifecycleBinding's start method:

                 

                   public void start() throws Exception
                   {
                      if (expr != null)
                         throw new IllegalArgumentException("The 'expr' attribute has been deprecated. Only the name of an annotation in the 'classes' attribute work now.");
                      if (classes == null)
                         throw new IllegalArgumentException("Null 'classes' attribute.");
                      if (manager != null)
                         log.warn("The use of manager has been deprecated and is unnecessary");
                      if (callbackBean == null)
                         throw new IllegalArgumentException("Null callback bean");
                      if (state == null)
                         throw new IllegalArgumentException("Null controller state");
                      if (name == null)
                         name = GUID.asString();
                
                      if (!classes.startsWith("@"))
                         throw new IllegalArgumentException("Could not parse '" + classes + " into an annotation. (It must start with '@')");
                      String annotationName = classes.substring(1);
                      Class<Annotation> clazz = null;
                      try
                      {
                         clazz = (Class<Annotation>)SecurityActions.getContextClassLoader().loadClass(annotationName);
                      }
                      catch (Exception e)
                      {
                         throw new IllegalArgumentException("An error occurred loading '" + classes + "'", e);
                      }
                
                      LifecycleAspectDependencyBuilderListItem item = new LifecycleAspectDependencyBuilderListItem(callbackBean, state, installMethod, uninstallMethod);
                      plugin = new LifecycleAnnotationPlugin<Annotation>(clazz, item);
                      getBeanAnnotationAdapter().addAnnotationPlugin(plugin);
                   }
                

                 

                So, in other words this is valid

                 

                   <lifecycle-configure xmlns="urn:jboss:aop-beans:1.0"
                               name="LifecycleCallback"
                               class="org.jboss.test.microcontainer.support.SimpleLifecycleCallback"
                               classes="@org.jboss.test.microcontainer.support.Lifecycle">
                          <property name="testProperty">Test123</property>               
                   </lifecycle-configure>
                 
                

                 

                But these are no longer valid:

                 

                   <lifecycle-configure xmlns="urn:jboss:aop-beans:1.0"
                               name="LifecycleCallback"
                               class="org.jboss.test.microcontainer.support.SimpleLifecycleCallback"
                               classes="org.jboss.test.microcontainer.support.SimpleBeanImpl">
                          <property name="testProperty">Test123</property>               
                   </lifecycle-configure>
                 
                

                 

                 

                   <lifecycle-configure xmlns="urn:jboss:aop-beans:1.0"
                               name="LifecycleCallback"
                               class="org.jboss.test.microcontainer.support.SimpleLifecycleCallback"
                               expr="class(org.jboss.test.microcontainer.support.SimpleBeanImpl)">
                          <property name="testProperty">Test123</property>               
                   </lifecycle-configure>
                 
                

                 

                 

                • 5. Re: Optimizing aop lifecycle
                  alesj

                  Anyway, the question I really had was: am I ok to expose the add/removeAnnotationPlugin() methods in the BeanAnnotationAdapter interface

                  No. :-)

                  That is useless impl detail, that shouldn't be part of BAA.

                   

                  This is what I would do

                  (1) expose Default/Common BAA as a bean, with callbacks on add/remove plugin (we might/should already be doing this)

                  (2) re-write <lifecycle-x> element to BeanMetaDataFactory handling to do config checks, and then produce LifecycleAnnotationPlugin bean

                   

                  This way (1) will pick up (2), w/o any need of knowing add/remove plugin impl detail

                  --> less code as you don't need that last few lines; start, stop, getBAA. ;-)

                  • 6. Re: Optimizing aop lifecycle
                    kabirkhan

                    So you mean something along the lines of

                     

                       <bean name="BeanAnnotationAdapterFactory" class="org.jboss.kernel.plugins.annotations.BeanAnnotationAdapterFactory">
                          <constructor factoryMethod="getInstance"/>
                       </bean>
                    
                       <bean name="BeanAnnotationAdapter" class="org.jboss.kernel.plugins.annotations.CommonAnnotationAdapter">
                          <constructor factoryMethod="getBeanAnnotationAdapter">
                             <factory bean="BeanAnnotationAdapterFactory"/>
                          </constructor>
                          <incallback method="addAnnotationPlugin"/>
                          <uncallback method="removeAnnotationPlugin"/>
                       </bean>
                     
                    
                    Now, what if (although probably unlikely) someone defines -Dorg.jboss.kernel.plugins.annotations.BeanAnnotationAdapter=some.class.without.these.Methods?
                    • 7. Re: Optimizing aop lifecycle
                      alesj
                      Now, what if (although probably unlikely) someone defines -Dorg.jboss.kernel.plugins.annotations.BeanAnnotationAdapter=some.class.without.these.Methods?

                      His/her problem. ;-)

                       

                      btw: you don't need to define class attribute in 2nd bean (at least I think so)

                      • 8. Re: Optimizing aop lifecycle
                        kabirkhan

                        This has been done.

                         

                        For the record, once we add this to AS we need to add the in/uncallbacks to BeanAnnotationAdapter in the bootstrap as shown here (Ales's suggestion worked):

                         

                           <bean name="BeanAnnotationAdapterFactory" class="org.jboss.kernel.plugins.annotations.BeanAnnotationAdapterFactory">
                              <constructor factoryMethod="getInstance"/>
                           </bean>
                        
                           <bean name="BeanAnnotationAdapter">
                              <constructor factoryMethod="getBeanAnnotationAdapter">
                                 <factory bean="BeanAnnotationAdapterFactory"/>
                              </constructor>
                              <incallback method="addAnnotationPlugin"/>
                              <uncallback method="removeAnnotationPlugin"/>
                           </bean>