14 Replies Latest reply on Feb 20, 2009 3:00 PM by alesj

    Locks on MainDeployerImpl

    alesj

      It looks like I over did it with introducing write lock on
      MainDeployerImpl::process (which now creates deployment contexts).

      The deadlock can be seen here:
      - https://jira.jboss.org/jira/browse/JBAS-6529
      - http://lists.jboss.org/pipermail/jboss-development/2009-February/013693.html

      My idea was to let others have read lock
      - the one's that are adding/removing on already concurrent maps,
      while the one's that actually process things should have a write lock.

      Is this really good/necessary?
      Or what would be the right way to have these locks?

      I used to have just write lock on shutdown.
      Which I guess could result in the same lock Brian sees
      if he would apply similar construct on the shutdown?

        • 1. Re: Locks on MainDeployerImpl
          alesj

          I guess if I just write lock the deploy and undeploy lists processing, you should be fine.

          Index: deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java
          ===================================================================
          --- deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java (revision 84459)
          +++ deployers-impl/src/main/java/org/jboss/deployers/plugins/main/MainDeployerImpl.java (working copy)
          @@ -322,33 +322,41 @@
          @@ -623,13 +631,13 @@
           if (deployers == null)
           throw new IllegalStateException("No deployers");
          
          + List<DeploymentContext> undeployContexts = null;
           lockWrite();
           try
           {
           if (shutdown.get())
           throw new IllegalStateException("The main deployer is shutdown");
          
          - List<DeploymentContext> undeployContexts = null;
           if (undeploy.isEmpty() == false)
           {
           // Undeploy in reverse order (subdeployments first)
          @@ -640,21 +648,33 @@
           Collections.sort(undeployContexts, reverted);
           undeploy.clear();
           }
          - if (undeployContexts != null)
          - {
          - deployers.process(null, undeployContexts);
          - }
          + }
          + finally
          + {
          + unlockWrite();
          + }
          
          - try
          - {
          - processToDeploy();
          - }
          - catch (DeploymentException e)
          - {
          - throw new RuntimeException("Error while processing new deployments", e);
          - }
          + if (undeployContexts != null)
          + {
          + deployers.process(null, undeployContexts);
          + }
          
          - List<DeploymentContext> deployContexts = null;
          + try
          + {
          + processToDeploy();
          + }
          + catch (DeploymentException e)
          + {
          + throw new RuntimeException("Error while processing new deployments", e);
          + }
          +
          + List<DeploymentContext> deployContexts = null;
          + lockWrite();
          + try
          + {
          + if (shutdown.get())
          + throw new IllegalStateException("The main deployer is shutdown");
          +
           if (deploy.isEmpty() == false)
           {
           deployContexts = new ArrayList<DeploymentContext>(deploy);
          @@ -662,15 +682,16 @@
           Collections.sort(deployContexts, comparator);
           deploy.clear();
           }
          - if (deployContexts != null)
          - {
          - deployers.process(deployContexts, null);
          - }
           }
           finally
           {
           unlockWrite();
           }
          +
          + if (deployContexts != null)
          + {
          + deployers.process(deployContexts, null);
          + }
           }
          
           public DeploymentStage getDeploymentStage(String deploymentName) throws DeploymentException
          


          • 2. Re: Locks on MainDeployerImpl
            alesj

             

            "alesj" wrote:
            I guess if I just write lock the deploy and undeploy lists processing, you should be fine.

            This is not fully sufficient either,
            as one could do addDeployment in between the two write locks in process(),
            and if this new deployment would actually be re-deploy,
            we would only pick up its deploy part, but not the undeploy part. :-(


            • 3. Re: Locks on MainDeployerImpl

               

              "alesj" wrote:
              "alesj" wrote:
              I guess if I just write lock the deploy and undeploy lists processing, you should be fine.

              This is not fully sufficient either,
              as one could do addDeployment in between the two write locks in process(),
              and if this new deployment would actually be re-deploy,
              we would only pick up its deploy part, but not the undeploy part. :-(


              No. The process() uses snapshots of the to(Un)Deploy.
              If two threads concurrently do:

              add(remove)Deployment()
              process()

              then they might end up processing each other's stuff but they won't miss one.

              • 4. Re: Locks on MainDeployerImpl

                As commented on the jboss-development list, only shutdown should be using the
                write lock.

                • 5. Re: Locks on MainDeployerImpl
                  alesj

                   

                  "adrian@jboss.org" wrote:

                  then they might end up processing each other's stuff but they won't miss one.

                  I wasn't thinking about missing one.
                  I had this in mind (#t = #thread):

                  1t - addDeployment
                  1t - process

                  While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
                  Meaning it will add its previous context to undeploy, which wouldn't be
                  processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
                  In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
                  which can lead to whatever. :-)

                  • 6. Re: Locks on MainDeployerImpl
                    alesj

                     

                    "alesj" wrote:
                    "adrian@jboss.org" wrote:

                    then they might end up processing each other's stuff but they won't miss one.

                    I wasn't thinking about missing one.
                    I had this in mind (#t = #thread):

                    1t - addDeployment
                    1t - process

                    While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
                    Meaning it will add its previous context to undeploy, which wouldn't be
                    processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
                    In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
                    which can lead to whatever. :-)

                    I probably have to create similar undeploy processing
                    (and not do that in addDeployment)
                    in process() as I do for deploy in processToDeploy.


                    • 7. Re: Locks on MainDeployerImpl

                       

                      "alesj" wrote:
                      "adrian@jboss.org" wrote:

                      then they might end up processing each other's stuff but they won't miss one.

                      I wasn't thinking about missing one.
                      I had this in mind (#t = #thread):

                      1t - addDeployment
                      1t - process

                      While 1t is just done with process' undeploy part, but before it enters processTopDeploy, 2t does addDeployment, but it's really re-deploy.
                      Meaning it will add its previous context to undeploy, which wouldn't be
                      processed until 2t calls process, but itself would be picked-up by 1t's processToDeploy.
                      In this case the 2t's re-deploy's undeploy wouldn't happen, only deploy,
                      which can lead to whatever. :-)


                      Ok, I see your point, but that's not a reason to use the write lock which is for a different purpose.

                      I can see two possible fixes:

                      1) Have a seperate toRedeploy list you can snapshot where "processUndeploy()"
                      adds those to the "toUndeploy" snapshot and creates an entry in the real "toDeploy" list.

                      2) Don't move a "toDeploy' request into the snapshot while it exists in the
                      real "toUndeploy" list.

                      • 8. Re: Locks on MainDeployerImpl

                         

                        "adrian@jboss.org" wrote:

                        I can see two possible fixes:

                        1) Have a seperate toRedeploy list you can snapshot where "processUndeploy()"
                        adds those to the "toUndeploy" snapshot and creates an entry in the real "toDeploy" list.

                        2) Don't move a "toDeploy' request into the snapshot while it exists in the
                        real "toUndeploy" list.


                        However, I guess those also contain potential race conditions.
                        The main issue being that the snapshoting of toDeploy and toUndeploy are not
                        in an atomic block together.

                        So a more complete fix would be to have just one list of "deployment actions"
                        that you can snapshot and then sort those actions such that the undeploys come first
                        in the snapshot list.

                        • 9. Re: Locks on MainDeployerImpl
                          alesj

                          What about if I just simply do this:

                           public void process()
                           {
                           if (deployers == null)
                           throw new IllegalStateException("No deployers");
                          
                           lockRead();
                           try
                           {
                           if (shutdown.get())
                           throw new IllegalStateException("The main deployer is shutdown");
                          
                           Map<String, Deployment> copy = new LinkedHashMap<String, Deployment>(toRedeploy);
                           toRedeploy.clear();
                          
                           try
                           {
                           processToUndeploy(copy.keySet());
                           }
                           catch (DeploymentException e)
                           {
                           throw new RuntimeException("Error while removing re-deployments", e);
                           }
                          
                           List<DeploymentContext> undeployContexts = null;
                           if (undeploy.isEmpty() == false)
                           {
                           // Undeploy in reverse order (subdeployments first)
                           undeployContexts = new ArrayList<DeploymentContext>(undeploy.size());
                           for (int i = undeploy.size() - 1; i >= 0; --i)
                           undeployContexts.add(undeploy.get(i));
                           if (reverted != null)
                           Collections.sort(undeployContexts, reverted);
                           undeploy.clear();
                           }
                          
                           if (undeployContexts != null)
                           {
                           deployers.process(null, undeployContexts);
                           }
                          
                           try
                           {
                           processToDeploy(copy.values());
                           }
                           catch (DeploymentException e)
                           {
                           throw new RuntimeException("Error while adding -redeployments", e);
                           }
                          
                           List<DeploymentContext> deployContexts = null;
                           if (deploy.isEmpty() == false)
                           {
                           deployContexts = new ArrayList<DeploymentContext>(deploy);
                           if (comparator != null)
                           Collections.sort(deployContexts, comparator);
                           deploy.clear();
                           }
                          
                           if (deployContexts != null)
                           {
                           deployers.process(deployContexts, null);
                           }
                           }
                           finally
                           {
                           unlockRead();
                           }
                           }
                          


                          Where I make a copy of currently existing re-deployments.
                          Then I don't care if some come in between.

                          Or am I missing something?

                          • 10. Re: Locks on MainDeployerImpl
                            brian.stansberry

                            A nit; you can lose changes between the copy and the clear. Better is:

                            Map<String, Deployment> copy = null;
                            synchronized(toRedeploy)
                            {
                             copy = new LinkedHashMap<String, Deployment>(toRedeploy);
                             toRedeploy.clear();
                            }
                            


                            • 11. Re: Locks on MainDeployerImpl
                              alesj

                              Shouldn't then any usage of toRedeploy be in synchronized(toRedeploy) block?

                              • 12. Re: Locks on MainDeployerImpl
                                brian.stansberry

                                No, you don't need to because you are using Collections.synchronizedMap. The map it returns synchronizes on itself in all methods to prevent concurrent access. So synchronizing on it will prevent other threads mutating it in the middle of the two calls.

                                There's some obscure aspect to this I won't go into details here (unless you want), but for reference, this is discussed in detail in section 5.1.1 of Java Concurrency in Practice.

                                You could make toRedeploy a plain HashMap and then put all the calls on it in a synchronized block. That would have the same effect and would be more understandable.

                                • 13. Re: Locks on MainDeployerImpl
                                  brian.stansberry

                                  For the record; I wasn't clear above; you don't need to synchronize in the other places toRedeploy is used because only a single call is made. If in any place you made 2 calls that were meant to be a single unit of work you would need to synchronize on the map.

                                  • 14. Re: Locks on MainDeployerImpl
                                    alesj

                                    That's what I meant. :-)