6 Replies Latest reply on Apr 23, 2007 8:39 AM by Ales Justin

    New Callback and error handling

    Adrian Brock Master

      I haven't looked at the new Callback code (InstallItems) in any detail yet,
      but I noticed immediately that the error handling is insufficient and can lead to inconsistent
      behaviour/state.

      Take the old implement of incrementState()

      try
      {
       install();
       updateInternalStateWithSuccess();
      }
      catch (Throwable t)
      {
       error = t;
      }
      finally
      {
       handleErrorHere(); // Invokes uninstall() back to NOT_INSTALLED (then *ERROR*)
      }
      


      This code is consistent. It either works and updates the state or if the error
      flag has been set it always returns the context to "not installed".

      Now we have:

      try
      {
       install();
       updateInternalStateWithSuccess();
       handleCallbacks(); // NEW Code
      }
      catch (Throwable t)
      {
       error = t;
      }
      finally
      {
       handleErrorHere();
      }
      


      The uninstall() includes the unwinding of the new callbacks.

      This looks ok? Although it is potentially lazy code. I haven't checked all the codepaths,
      to see if it really works.

      Better would be:

      try
      {
       install();
       updateInternalStateWithSuccess();
       try
       {
       handleCallbacks(); // NEW Code
       }
       catch (Throwable t)
       {
       error = t;
       }
       finally
       {
       // Reverse the already succesful install()
       }
      }
      catch (Throwable t)
      {
       error = t;
      }
      finally
      {
       handleErrorHere();
      }
      


      Although I think uninstall() is potentially doing this anyway, I'd be more confident
      of that fact if the there was a nested try/catch block.

      The real problem is the uninstall(). It has two problems.

      1) It should be asymmetrical with the install()

      // Reverse order of the install()!
      removeCallbacksFirst();
      context.uninstall();
      


      2) It really does need multiple try/catch blocks.

      uninstall() should ATTEMPT to reverse all the changes of install()

      Currently it is not doing this.

      Previous code:
      try
      {
       context.uninstall();
      }
      catch (Throwable t)
      {
       // If the callout doesn't work, at least we tried.
       // But continue as it if it did otherwise the state will be inconsistent.
      }
      


      New Code:
      try
      {
       context.uninstall();
       removeCallbacks();
      }
      catch (Throwable t)
      {
       // An error in uninstall() will come here meaning callbacks are not removed!
      }
      


      This code reminds me of the horribly poor implementation of a State Machine
      in the old ServiceController where I went through and fixed the most obvious
      problems with the error handling a few years ago.

      Let's not make the same mistakes with the new Microcontainer! :-)

        • 1. Re: New Callback and error handling
          Adrian Brock Master

          The code really needs to be refactored such that the core install and
          before/after methods (the callbacks currently implement only the after pattern)
          are handling atomically and consistently.

          e.g. In the pseudo code I posted above:

          1) An error in one of the callbacks needs to unwind any previously successful callbacks
          already invoked (not all of them) and then do the main uninstall()

          2) If there is an error in the main install(), we don't even want to try to reverse the callbacks
          since they were never invoked in the first place.

          You can find an example of this kind of error handling in the MainDeployer/DeployerWrapper.

          • 2. Re: New Callback and error handling
            Adrian Brock Master

            Finally on the main callback code (not what I've shown above but it has the same
            problems!), its questionable whether an error in a callback should stop the main bean
            from being installed anyway?

            This potentially represents a DOS attack!

            @Install
            public void dosAttack(DataSource ds)
            {
             throw new Error("Nobody can register a DataSource!");
            }
            


            An error in a callback should just be logged as a warning about the broken
            callback, not stop the original referenced service from being installed.

            • 3. Re: New Callback and error handling
              Ales Justin Master

               

              "adrian@jboss.org" wrote:
              I haven't looked at the new Callback code (InstallItems) in any detail yet,
              but I noticed immediately that the error handling is insufficient and can lead to inconsistent behaviour/state.

              Yup, I was waiting for some input, that's why I didn't close the JIRA task yet. ;-)

              "adrian@jboss.org" wrote:

              1) It should be asymmetrical with the install()

              // Reverse order of the install()!
              removeCallbacksFirst();
              context.uninstall();
              


              Yes, I though of that.
              But I need the uninstalling context to actually change its state.
              For the CollectionCallbackItem to get its collection right on the uninstall phase - not picking up the current uninstalling context.
              But I'll try fix this.

              "adrian@jboss.org" wrote:

              Let's not make the same mistakes with the new Microcontainer! :-)

              :-)
              Agreed.
              Will add the necessary error handling.

              • 4. Re: New Callback and error handling
                Adrian Brock Master

                P.S. When I say "pseudo code" like I did here:
                http://www.jboss.com/index.html?module=bb&op=viewtopic&t=105187

                It means I've omitted things like error handling, logging and the real implementation
                details (which are usually more involved), etc. for clarity.

                You should not omit them in the real code! :-)

                • 5. Re: New Callback and error handling
                  Ales Justin Master

                   

                  "adrian@jboss.org" wrote:
                  P.S. When I say "pseudo code" like I did here:
                  http://www.jboss.com/index.html?module=bb&op=viewtopic&t=105187

                  It means I've omitted things like error handling, logging and the real implementation
                  details (which are usually more involved), etc. for clarity.

                  You should not omit them in the real code! :-)


                  I already hacked more than 100 classes, so if there was already all the error, logging, ... code commited in as well, I doubt your hard discs could take it. :-)

                  • 6. Re: New Callback and error handling
                    Ales Justin Master

                     

                    "adrian@jboss.org" wrote:

                    An error in a callback should just be logged as a warning about the broken
                    callback, not stop the original referenced service from being installed.


                    Now all callback invocation error's are suppressed - meaning that context proceeds to next state anyway - if dependencies (cardinality dependency, ...) are satisfied.
                    I also changed the order on the uninstall to be asymmetric.

                    Is this now OK?
                    Or should I add additional error, log, ... handling?