7 Replies Latest reply on Jun 5, 2007 12:26 PM by kabirkhan

    New Lifecycle aspects is wrong

      This relates to this long discussion thread:
      http://www.jboss.com/index.html?module=bb&op=viewtopic&t=101253

      I finally got chance to look at what has been done for this and it is wrong.

      1) Type safety

      I quite like the idea of the BeanInfo.getDependencies()
      returning a builder rather than just names so it can be called back
      with additional context on what it needs to work on.

      But this needs to be type safe.
      e.g. This code shows the problem (the cast - which isn't even checked properly!)

       // add custom dependencies (e.g. AOP layer).
       List<Object> dependencies = info.getDependencies(md);
       log.trace("Extra dependencies for " + context.getName() + " " + dependencies);
       if (dependencies != null)
       {
       for (Object dependencyItem : dependencies)
       {
       ((DependencyBuilderListItem)dependencyItem).addDependency(context);
       }
       }
      


      2) BeanMetaData is not a place to store runtime information
      that is what the ControllerContext is for.

      You should not be modifying the BeanMetaData. This is information
      that the user/client is requesting. Any additional information runtime information
      should be stored in the ControllerContext, i.e. linked in via DependencyInfo/Item

      This is how the initial runtime/additional depedencies were coded and that rule
      must be maintained.

      Ales also made the same mistake, so I guess we need a big warning
      message on BeanMetaData to stop people adding things to the model
      that are not declartive (intended to be declared by the user)? :-)

      3) Too much of this implementation detail is being leaked into spi.

      e.g. the KernelControllerContext interface
       /**
       * Get the lifecycle callbacks for a particular state.
       *
       * @param state the state callbacks refer to
       * @return List<LifecycleCallbackMetaData>
       */
       List<LifecycleCallbackMetaData> getLifecycleCallbacks(ControllerState state);
      


      Besides what I said on the other thread that this is too POJO specifc,
      it is also an implementation detail that should not need to be exposed on the interface.

      This is also something else that I've tried to maintain in the Microcontainer api.

      If the processing is generic state that is going to be useful for all implementations
      of an interface then it belongs on thatinterface, Otherwise it should be
      a method specific to an implementation and the integration should use
      "private api". e.g. I wouldn't have a problem if this was on the
      Abstract[Kernel]ControllerContext.

        • 1. Re: New Lifecycle aspects is wrong

          Don't get me wrong.
          Overall, I think what this is trying to achieve is correct and I like the idea
          of a builder.

          But it has been targetted too specifically at solving a problem in one particular context
          and this is coupled with a misunderstanding of what each part of the model is intended to do.

          The ideas need abstracting into a context neutral api (not just POJO)
          and implementations details removed from the spi.

          • 2. Re: New Lifecycle aspects is wrong
            alesj

             

            "adrian@jboss.org" wrote:

            Ales also made the same mistake, so I guess we need a big warning
            message on BeanMetaData to stop people adding things to the model
            that are not declartive (intended to be declared by the user)? :-)

            Where?
            At the introduction of BeanMetaData being ValueMetaData?
            That's still a work in progress. :-)

            • 3. Re: New Lifecycle aspects is wrong
              kabirkhan

              Point 1) has been done. I will attempt to address 2) and 3) next week

              • 4. Re: New Lifecycle aspects is wrong
                kabirkhan

                Lifecycle callback aspects have been redone. They are now configured via DependencyInfo rather than via BeanMetaData. The aspect install/uninstall methods now take a ControllerContext instead of a KernelControllerContext, and the calls to the chain are driven by AbstractController rather than KernelControllerContextAction.

                It seems a bit weird that getMetaData() only exists in "POJO mode" (in KernelControllerContext) so that lifecycle callback aspects need to cast the ControllerContext to KernelControllerContext if they want to access the metadata (e.g. JMXLifecyleCallbacl). But then again I guess not all types of context will have metadata?

                • 5. Re: New Lifecycle aspects is wrong
                  alesj

                  Maybe some unwinding on the exception in install phase could be considered.
                  From my callback experience:
                  - http://www.jboss.com/index.html?module=bb&op=viewtopic&t=107063

                  • 6. Re: New Lifecycle aspects is wrong
                    kabirkhan

                    This has been added. I originally modified AbstractController, but found that the callbacks were being "uninstall"ed twice, so it does its own unwinding. I've added some checks to AbstractLifecycleCallbackItem to check if the "install" callback was called for a particular callback, and if not to skip the "uninstall" step for that.

                    One borderline case is the lifecycle callback whose "install" method caused an error, should the callback for that be called or not? I think so, since it might have had partial success. See UnwindLifecycleTestCase for details.

                    • 7. Re: New Lifecycle aspects is wrong
                      kabirkhan