8 Replies Latest reply on Jul 30, 2010 12:31 PM by jaikiran

    AbstractRealDeployerWithInput fails to call undeploy() on DeploymentVisitor, for failed deployments

    jaikiran

      While investigating this issue http://community.jboss.org/message/555195#555195, I have stumbled upon a MC deployer bug. Here's an simplified version of what's happening:

       

      public class MyDeployer extends AbstractRealDeployerWithInput<JBossEnterpriseBeanMetaData>
            implements
               DeploymentVisitor<JBossEnterpriseBeanMetaData>
               
      {
      
      ...
         @Override
         public void deploy(DeploymentUnit unit, JBossEnterpriseBeanMetaData beanMetaData) throws DeploymentException
         {
           doStepOne(); // like register component to some service name
           
           doStepTwo(); // can be any operation which ends up throwing an runtime exception
      
         }         
         
         @Override
         public void undeploy(DeploymentUnit unit, JBossEnterpriseBeanMetaData enterpriseBean)
         {
            undoStepOne(); // i.e. unregister the service
         }
         
      }
      
      

       

       

      As can be seen, the deploy() involves more than one step. The first step does registering of services (or some functionality internal to my deployer). The second step of deploy() does some other thing but ends up throwing an exception. I would expect that the undeploy() method
      would be called on MyDeployer for this (failed) deployment unit, so that I can undo/cleanup whatever was done in doStepOne() of deploy() operation.

       

      However, the undeploy() never gets called. Looking at MC's AbstractRealDeployerWithInput.deploy() method:

       

        protected <U> void deploy(DeploymentUnit unit, DeploymentVisitor<U> visitor) throws DeploymentException
         {
            if (visitor == null)
               throw new IllegalArgumentException("Null visitor.");
      
            List<U> visited = new ArrayList<U>();
            try
            {
               Set<? extends U> deployments = unit.getAllMetaData(visitor.getVisitorType());
               for (U deployment : deployments)
               {
                  visitor.deploy(unit, deployment);
                  visited.add(deployment);
               }
            }
            catch (Throwable t)
            {
               for (int i = visited.size()-1; i >= 0; --i)
               {
                  try
                  {
                     visitor.undeploy(unit, visited.get(i));
                  }
                  catch (Throwable ignored)
                  {
                     log.warn("Error during undeploy: " + unit.getName(), ignored);
                  }
               }
               throw DeploymentException.rethrowAsDeploymentException("Error deploying: " + unit.getName(), t);
            }
         }
         
      
      

       

      More specifically:

       

                  
                  visitor.deploy(unit, deployment);
                  visited.add(deployment);
      
      

       

      visitor.deploy(...) throws the exception and hence the deployment is never added to "visited" which effectively means that in the catch block:

       

      for (int i = visited.size()-1; i >= 0; --i)
      {
              try
              {
                 visitor.undeploy(unit, visited.get(i));
              }
              catch (Throwable ignored)
              {
                 log.warn("Error during undeploy: " + unit.getName(), ignored);
              }
      }
      
      

       

      The "visited" will never contain this failed deployment. Ultimately, the undeploy() will never get called for that failed deployment unit.

       

      Message was edited by: jaikiran pai - Fixed subject line

        • 1. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
          alesj

          This is expected.

          It means that the "deployment" wasn't successfully deployed.

          I would say it's the visitors fault that the current deployment doesn't get properly cleaned up after error.

          • 2. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
            jaikiran

            Ales Justin wrote:

             

            It means that the "deployment" wasn't successfully deployed.

            Yes, it wasn't successfully deployed and hence I would expect immediate undeploy()

             

            Ales Justin wrote:

             


            I would say it's the visitors fault that the current deployment doesn't get properly cleaned up after error.

            Consider this:

            deploy()
            {
              doMyWork();
            
              callThirdPartyCodeWhichCanEndUpThrowingThrowableOrRunTimeException();
            }
            

             

            Surely, my deployer/visitor isn't expected to catch Throwable and cleanup whatever I did in doMyWork(), or is it?

            • 3. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
              alesj
              Consider this:
              deploy()
              {
                doMyWork();

                callThirdPartyCodeWhichCanEndUpThrowingThrowableOrRunTimeException();
              }

               


              Surely, my deployer/visitor isn't expected to catch Throwable and cleanup whatever I did in doMyWork(), or is it?


              Sure it is.

              How on earth should I know what a "revert-doMyWork()" looks like? ;-)

               

              It should be then

               

              deploy()
              {
                doMyWork();
                 
                try { 
                   callThirdPartyCodeWhichCanEndUpThrowingThrowableOrRunTimeException();
                } catch (E e) {
                   revertDoMyWork();
                   throw e;        
                }
              }
              
              
              
              
              • 4. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
                jaikiran

                Ales Justin wrote:

                 

                Consider this:
                deploy()
                {
                  doMyWork();

                  callThirdPartyCodeWhichCanEndUpThrowingThrowableOrRunTimeException();
                }

                 


                Surely, my deployer/visitor isn't expected to catch Throwable and cleanup whatever I did in doMyWork(), or is it?


                Sure it is.

                How on earth should I know what a "revert-doMyWork()" looks like? ;-)

                You need not know that logic All that I expect is a undeploy() call on my visitor which failed during deploy. And my visitor's undeploy() will have the knowledge of revert-doMyWork();

                • 5. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
                  alesj
                  You need not know that logic All that I expect is a undeploy() call on my visitor which failed during deploy. And my visitor's undeploy() will have the knowledge of revert-doMyWork();

                  At the end it all depends on impl details; e.g. at which step is the deployment used enough so you can do proper revert

                  Our code assumes the least logic possible, meaning if the error occurred, you didn't do much, hence no need to actually do undeploy.

                  But you can simply "fix" this generic behavior by impling this logic into your custom visitor.

                  • 6. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
                    jaikiran

                    Ales Justin wrote:

                    But you can simply "fix" this generic behavior by impling this logic into your custom visitor.

                    That would effectively mean, that *every* implementation of the org.jboss.deployers.spi.deployer.helpers.DeploymentVisitor interface, has to have the following code in their deploy:

                     

                    deploy(DeploymentUnit unit, T deployment) throws DeploymentException;
                    {
                    
                       try
                       {
                         ...
                        }
                       catch (Throwable t)
                      {
                         this.undeploy(unit, deployment);
                         ...
                      }
                    }
                    
                    undeploy(DeploymentUnit unit, T deployment)
                    {
                      ...
                    }
                    
                    • 7. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
                      alesj
                      That would effectively mean, that *every* implementation of the org.jboss.deployers.spi.deployer.helpers.DeploymentVisitor interface, has to have the following code in their deploy:

                      No, why?

                       

                      See existing impl, none has that.

                      Either they don't have anything, since the deploy action is very simple (almost atomic ),

                      or they have some other additional undeploy logic; e.g. remove added components.

                       

                      The idea is that the visitor's deploy is very simple, or you missused the deployer.

                      • 8. Re: AbstractRealDeployerWithInput fails to call undeploy() on deployer, for failed deployments
                        jaikiran

                        Ales Justin wrote:

                         

                        That would effectively mean, that *every* implementation of the org.jboss.deployers.spi.deployer.helpers.DeploymentVisitor interface, has to have the following code in their deploy:

                        No, why?

                         

                        Because, in it's current form, if some *unexpected* exception occurs then the undeploy will never get called without that try{}catch(Throwable t) block.

                         

                         

                        Ales Justin wrote:

                         


                        See existing impl, none has that.

                        And all those will fail (i.e. their undeploy() will not get called) if they happen to run into runtime exceptions in their deploy.

                         

                         

                        Ales Justin wrote:


                        Either they don't have anything, since the deploy action is very simple

                         

                        Doesn't matter if it's simple or not Even if it's a couple of lines code, it can end up throwing a runtime.

                         

                        Ales Justin wrote:


                        (almost atomic ),

                        That's just an assumption

                         

                        Ales Justin wrote:

                         

                        The idea is that the visitor's deploy is very simple, or you missused the deployer.

                         

                        Consider this again, in my visitor's deploy:

                         

                        void deploy(DeploymentUnit unit, T deployment) throws DeploymentException
                        {
                            Ejb3Registry.register(...);
                        
                            callThirdPartyCodeWhichRunsIntoNullPointerException();
                        }  
                        

                         

                        I don't see any misuse here. So when I call the  callThirdPartyCodeWhichRunsIntoNullPointerException(), which as the name suggests will run into a NPE (for some reason), I have no way of unregistering from Ejb3Registry, unless I wrap all this in a try{}catch(Throwable t) block or at the very least in a try{}catch(RuntimeException re) block.