10 Replies Latest reply on Dec 2, 2010 3:24 PM by dmlloyd

    Deployer chain cleanup

    jason.greene

      Alexey posted a proposed patch here:

      https://github.com/aloubyansky/jboss-as/commit/fa92ebafc41905674d6143d461257d49ee40c965

       

      I had something similar in mind, but thought a single cleanup method on the chain that would use attachment state would be sufficient. Any thoughts on this?

        • 1. Re: Deployer chain cleanup
          dmlloyd

          In an ideal world we'd be able to use an interceptor-chain kind of pattern where each processor calls invokeNext() or whatever, and any error cleanup could be done in a finally block.  However the grim reality is that we could face stack exhaustion if we had a truly large number of extensions involved.  I think the next-best option is what you suggest: a cleanup() method which is called if a later processor fails.

           

          So to restate:

          1. Add a cleanup() method to DeploymentUnitProcessor
          2. Each implementation of DUP should handle failure cleanly, so processDeployment is essentially atomic: either everything gets done or nothing (no partial state when an exception is thrown).
          3. Then the implementation of cleanup() can perform a full cleanup safely, because it will only ever be called if processDeployment() had completed successfully and a later processDeployment() failed.
          4. This implies that if a given processDeployment() fails, its cleanup() is not called, but the cleanup() methods on all the DUPs before the one that failed are called.

           

          Make sense?

          • 2. Re: Deployer chain cleanup
            dmlloyd

            David Lloyd wrote:

             

            This implies that if a given processDeployment() fails, its cleanup() is not called, but the cleanup() methods on all the DUPs before the one that failed are called.

             

            I should elaborate here: the cleanup() methods should be called in reverse order, just like recursive catch/finally blocks would be.

            • 3. Re: Deployer chain cleanup
              brian.stansberry

              I like the approach of each DUP being responsible for cleaning itself up if it fails, with the cleanup call being reserved for those that had previously completed successfully. That's the way the handling of lists of model/runtime updates work.

               

              Putting the logic to do the cleanup work inside DeploymentChain.processDeployment() seems easiest -- no need to do any kind of external bookkeeping of what DUPs have been invoked and need cleanup. The contract for that method can guarantee that all cleanup will be done before any exception is thrown.

              • 4. Re: Deployer chain cleanup
                aloubyansky

                Adding a cleanup method to DUP is probably the most natural thing to do. The reason I didn't do it was I didn't want to go modify each DUP implementation.

                Also I had an idea of doing something like this

                 

                {code}// this is the interface I'm currently using

                public interface DeploymentRollbackAction {

                   void rollback(DeploymentUnitContext ctx);

                }

                 

                public abstract class DeploymentUnitProcessorWithRollback implements DeploymentUnitProcessor, DeploymentRollbackAction {


                    public final void processDeployment(DeploymentUnitContext context) throws DeploymentUnitProcessingException {

                        context.pushRollback(this);

                        doProcess(context);

                    }


                    protected abstract void doProcess(DeploymentUnitContext context) throws DeploymentUnitProcessingException;

                }{code}

                 

                So then DUP that may need cleaning up can extend this processor.

                • 5. Re: Deployer chain cleanup
                  thomas.diesler

                  David, how many extensions do you think would be needed to reach stack exhaustion? Its only a relativly small frame you would need per DUP, no?

                  • 6. Re: Deployer chain cleanup
                    aloubyansky

                    Actually, having looked at the processor implementations, there are just a few that need to do clean up. For example, in case of managed beans it's 1 out of 12. So, apparently, it's not that common.

                     

                    Another thought on this subject is that if we are to add some clean-up method then perhaps it makes sense to invoke it when the unit is undeployed. I.e. go through the same deployment chain and invoke clean-up on the processors.

                    • 7. Re: Deployer chain cleanup
                      dmlloyd

                      Thomas Diesler wrote:

                       

                      David, how many extensions do you think would be needed to reach stack exhaustion? Its only a relativly small frame you would need per DUP, no?

                       

                      Yes, theoretically it's a small frame, but if we have hundreds of processors it can add up.  If someone told me that they can successfully recurse, say, 2000 times with, say, 10 local reference-type variables per frame and they tested it on Windows, Mac, Linux (x86 and others) and Solaris with default VM options, then I might be persuaded though.

                      • 8. Re: Deployer chain cleanup
                        dmlloyd

                        Alexey Loubyansky wrote:

                         

                        Actually, having looked at the processor implementations, there are just a few that need to do clean up. For example, in case of managed beans it's 1 out of 12. So, apparently, it's not that common.

                         

                        Another thought on this subject is that if we are to add some clean-up method then perhaps it makes sense to invoke it when the unit is undeployed. I.e. go through the same deployment chain and invoke clean-up on the processors.

                         

                        No, undeploy will be a different situation: the batch builders will be gone, for example, and so will much of the deploy-time metainformation.  In addition, certain things like the deployment mount(s) will be associated with a service rather than the deployment unit processor context.  Undeploy should consist of nothing more than iterating all services belonging to a deployment unit and setting their mode to "REMOVE", and then waiting for completion.

                        • 9. Re: Deployer chain cleanup
                          dmlloyd

                          David Lloyd wrote:

                           

                          No, undeploy will be a different situation....

                           

                          OK turns out I'm probably completely wrong about this.  I'll follow up later with an email to the as7-dev list to explain...

                          • 10. Re: Deployer chain cleanup
                            dmlloyd