Fixed RuleService concurrency around enable/disable at NodeRef level

- Done while rolling in ALF-10839: Eliminate rule discovery overhead on property update when rules have been disabled
 - Some checking of rule state done BEFORE walking up the node hierarchy
 - Also fixes ALF-4216: disabledRules List is not thread safe


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@31255 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

config/alfresco/rule-services-context.xml

@@ -91,27 +91,16 @@
    <!-- Rule triggers -->
    
    <bean id="rule-trigger-base" abstract="true" init-method="registerRuleTrigger">
-      <property name="policyComponent">
+      <property name="policyComponent" ref="policyComponent"/>
-         <ref bean="policyComponent"/>
+      <property name="nodeService" ref="nodeService"/>
-      </property>
+      <property name="contentService" ref="contentService"/>
-      <property name="nodeService">
+      <property name="authenticationComponent" ref="authenticationComponent"/>
-         <ref bean="nodeService"/>
+      <property name="dictionaryService" ref="dictionaryService"/>
-      </property>
+      <property name="ruleService" ref="ruleService"/>
-      <property name="contentService">
-         <ref bean="contentService"/>
-      </property>
-      <property name="authenticationComponent">
-         <ref bean="authenticationComponent"/>
-      </property>
-      <property name="dictionaryService">
-         <ref bean="dictionaryService"/>
-      </property>
    </bean>
    
    <bean id="on-create-node-trigger" class="org.alfresco.repo.rule.ruletrigger.CreateNodeRuleTrigger" parent="rule-trigger-base">
-      <property name="ruleService">
+      <property name="runtimeRuleService" ref="ruleService"/>
-          <ref bean="ruleService"/>
-      </property>
       <property name="isClassBehaviour">
            <value>true</value>
       </property>

source/java/org/alfresco/repo/rule/RuleServiceCoverageTest.java

@@ -925,7 +925,7 @@ public class RuleServiceCoverageTest extends TestCase
         
         this.ruleService.saveRule(this.nodeRef, rule);
 
-        MailActionExecuter mailService = (MailActionExecuter) ((ApplicationContextFactory) this.applicationContext
+        MailActionExecuter mailService = (MailActionExecuter) ((ApplicationContextFactory) applicationContext
                     .getBean("OutboundSMTP")).getApplicationContext().getBean("mail");
         mailService.setTestMode(true);
         mailService.clearLastTestMessage();
@@ -965,7 +965,7 @@ public class RuleServiceCoverageTest extends TestCase
 
         String illegalName = "MyName.txt "; // space at end
         
-        MailActionExecuter mailService = (MailActionExecuter) ((ApplicationContextFactory) this.applicationContext
+        MailActionExecuter mailService = (MailActionExecuter) ((ApplicationContextFactory) applicationContext
                     .getBean("OutboundSMTP")).getApplicationContext().getBean("mail");
         mailService.setTestMode(true);
         mailService.clearLastTestMessage();
@@ -1400,27 +1400,44 @@ public class RuleServiceCoverageTest extends TestCase
         		null);
         
         this.ruleService.saveRule(this.nodeRef, rule);
-        this.ruleService.disableRules(this.nodeRef);
 
-        NodeRef newNodeRef = this.nodeService.createNode(
+        RetryingTransactionCallback<NodeRef> noRulesWork = new RetryingTransactionCallback<NodeRef>()
-                this.nodeRef,
+        {
+            @Override
+            public NodeRef execute() throws Throwable
+            {
+                ruleService.disableRules(nodeRef);
+                
+                NodeRef newNodeRef = nodeService.createNode(
+                        nodeRef,
                         ContentModel.ASSOC_CHILDREN,                
                         QName.createQName(TEST_NAMESPACE, "children"),
                         ContentModel.TYPE_CONTENT,
                         getContentProperties()).getChildRef();         
                 addContentToNode(newNodeRef);
-        assertFalse(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));      
+                return newNodeRef;
+            }
+        };
+        NodeRef newNodeRef = transactionService.getRetryingTransactionHelper().doInTransaction(noRulesWork);
+        assertFalse(nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));      
 
-        this.ruleService.enableRules(this.nodeRef);
+        RetryingTransactionCallback<NodeRef> withRulesWork = new RetryingTransactionCallback<NodeRef>()
-        
+        {
-        NodeRef newNodeRef2 = this.nodeService.createNode(
+            @Override
-                this.nodeRef,
+            public NodeRef execute() throws Throwable
+            {
+                NodeRef newNodeRef2 = nodeService.createNode(
+                        nodeRef,
                         ContentModel.ASSOC_CHILDREN,                
                         QName.createQName(TEST_NAMESPACE, "children"),
                         ContentModel.TYPE_CONTENT,
                         getContentProperties()).getChildRef();        
                 addContentToNode(newNodeRef2);
-        assertTrue(this.nodeService.hasAspect(newNodeRef2, ContentModel.ASPECT_VERSIONABLE));       
+                return newNodeRef2;
+            }
+        };
+        NodeRef newNodeRef2 = transactionService.getRetryingTransactionHelper().doInTransaction(withRulesWork);
+        assertTrue(nodeService.hasAspect(newNodeRef2, ContentModel.ASPECT_VERSIONABLE));
     }
     
     /**

source/java/org/alfresco/repo/rule/RuleServiceImpl.java

@@ -40,6 +40,7 @@ import org.alfresco.repo.policy.PolicyComponent;
 import org.alfresco.repo.security.authentication.AuthenticationUtil;
 import org.alfresco.repo.transaction.AlfrescoTransactionSupport;
 import org.alfresco.repo.transaction.TransactionListener;
+import org.alfresco.repo.transaction.TransactionalResourceHelper;
 import org.alfresco.service.cmr.action.Action;
 import org.alfresco.service.cmr.action.ActionService;
 import org.alfresco.service.cmr.action.ActionServiceException;
@@ -79,6 +80,12 @@ public class RuleServiceImpl
                 NodeServicePolicies.OnUpdateNodePolicy,
                 NodeServicePolicies.OnAddAspectPolicy
 {
+    /** key against which to store disabled rule types in the current txn */
+    private static final String KEY_DISABLED_RULE_TYPES = "RuleServiceImpl.disabledRuleTypes";
+    
+    /** key against which to store disabled rule nodes in the current txn */
+    private static final String KEY_DISABLED_RULE_NODES = "RuleServiceImpl.disabledRuleNodes";
+    
     /** key against which to store rules pending on the current transaction */
     private static final String KEY_RULES_PENDING = "RuleServiceImpl.PendingRules";
     
@@ -116,23 +123,12 @@ public class RuleServiceImpl
      */
     private SimpleCache<NodeRef, List<Rule>> nodeRulesCache;
        
-    /**
-     * List of disabled node refs.  The rules associated with these nodes will node be added to the pending list, and
-     * therefore not fired.  This list is transient.
-     * 
-     * TODO: (DH) Make this txn-local
-     */
-    private Set<NodeRef> disabledNodeRefs = new HashSet<NodeRef>(5);
-    
     /**
      * List of disabled rules.  Any rules that appear in this list will not be added to the pending list and therefore
      * not fired.
      */
     private Set<Rule> disabledRules = new HashSet<Rule>(5);
     
-    /** List of disables rule types */
-    private Set<String> disabledRuleTypes = new HashSet<String>(3);
-
     /**
      * All the rule type currently registered
      */
@@ -388,21 +384,22 @@ public class RuleServiceImpl
     @Override
     public boolean rulesEnabled(NodeRef nodeRef)
     {
-        return (this.disabledNodeRefs.contains(nodeRef) == false);
+        Set<NodeRef> disabledRuleNodes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_NODES);
+        return !disabledRuleNodes.contains(nodeRef);
     }
 
     @Override
     public void disableRules(NodeRef nodeRef)
     {
-        // Add the node to the set of disabled nodes
+        Set<NodeRef> disabledRuleNodes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_NODES);
-        this.disabledNodeRefs.add(nodeRef);
+        disabledRuleNodes.add(nodeRef);
     }
 
     @Override
     public void enableRules(NodeRef nodeRef)
     {
-        // Remove the node from the set of disabled nodes
+        Set<NodeRef> disabledRuleNodes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_NODES);
-        this.disabledNodeRefs.remove(nodeRef);
+        disabledRuleNodes.remove(nodeRef);
     }
     
     @Override
@@ -420,24 +417,22 @@ public class RuleServiceImpl
     @Override
     public void disableRuleType(String ruleType)
     {
+        Set<String> disabledRuleTypes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_TYPES);
     	disabledRuleTypes.add(ruleType);
     }
 
     @Override
     public void enableRuleType(String ruleType)
     {
+        Set<String> disabledRuleTypes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_TYPES);
         disabledRuleTypes.remove(ruleType);
     }
     
     @Override
     public boolean isRuleTypeEnabled(String ruleType)
     {
-    	boolean result = true;
+        Set<String> disabledRuleTypes = TransactionalResourceHelper.getSet(KEY_DISABLED_RULE_TYPES);
-    	if (disabledRuleTypes.contains(ruleType) == true)
+        return !disabledRuleTypes.contains(ruleType);
-    	{
-    		result = false;    		
-    	}
-    	return result;
     }
     
     @Override
@@ -1021,7 +1016,7 @@ public class RuleServiceImpl
         
         // First check to see if the node has been disabled
         if (this.isEnabled() == true &&
-            this.disabledNodeRefs.contains(this.getOwningNodeRef(rule)) == false &&
+            this.rulesEnabled(this.getOwningNodeRef(rule)) &&
             this.disabledRules.contains(rule) == false)
         {
             PendingRuleData pendingRuleData = new PendingRuleData(actionableNodeRef, actionedUponNodeRef, rule, executeAtEnd);

source/java/org/alfresco/repo/rule/RuleTypeImpl.java

@@ -20,8 +20,6 @@ package org.alfresco.repo.rule;
 
 import java.util.List;
 
-import org.springframework.extensions.surf.util.I18NUtil;
-import org.alfresco.model.ContentModel;
 import org.alfresco.repo.action.CommonResourceAbstractBase;
 import org.alfresco.repo.rule.ruletrigger.RuleTrigger;
 import org.alfresco.service.cmr.repository.NodeRef;
@@ -31,6 +29,7 @@ import org.alfresco.service.cmr.rule.RuleService;
 import org.alfresco.service.cmr.rule.RuleType;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.springframework.extensions.surf.util.I18NUtil;
 
 /**
  * Rule type implementation class.
@@ -121,9 +120,6 @@ public class RuleTypeImpl extends CommonResourceAbstractBase implements RuleType
     {
         if (ruleService.isEnabled() == true &&
             nodeService.exists(actionedUponNodeRef) == true && 
-            nodeService.hasAspect(actionedUponNodeRef, ContentModel.ASPECT_TEMPORARY) == false &&
-            // Temporary workaround to prevent rules running on cm:rating nodes (which happened for 'liked' folders ALF-8308 & ALF-8382)
-            ContentModel.TYPE_RATING.equals(nodeService.getType(actionedUponNodeRef)) == false &&
             ruleService.isRuleTypeEnabled(this.getName()) == true)
         {
             List<Rule> rules = ruleService.getRules(

source/java/org/alfresco/repo/rule/ruletrigger/BeforeDeleteChildAssociationRuleTrigger.java

@@ -82,6 +82,12 @@ public class BeforeDeleteChildAssociationRuleTrigger
 
     public void beforeDeleteChildAssociation(ChildAssociationRef childAssocRef)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
+
         NodeRef childNodeRef = childAssocRef.getChildRef();
 
         // Avoid renamed nodes

source/java/org/alfresco/repo/rule/ruletrigger/CreateNodeRuleTrigger.java

@@ -59,7 +59,7 @@ public class CreateNodeRuleTrigger extends RuleTriggerAbstractBase
     private boolean isClassBehaviour = false;
 	
     /** Runtime rule service */
-	RuntimeRuleService ruleService;
+	RuntimeRuleService runtimeRuleService;
     
 	/**
 	 * Set whether this is a class behaviour or not
@@ -72,9 +72,9 @@ public class CreateNodeRuleTrigger extends RuleTriggerAbstractBase
     /**
      * Set the rule service
      */
-    public void setRuleService(RuntimeRuleService ruleService)
+    public void setRuntimeRuleService(RuntimeRuleService runtimeRuleService)
     {
-        this.ruleService = ruleService;
+        this.runtimeRuleService = runtimeRuleService;
     }
     
     /**
@@ -113,6 +113,11 @@ public class CreateNodeRuleTrigger extends RuleTriggerAbstractBase
      */
     public void onCreateNode(ChildAssociationRef childAssocRef)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         NodeRef nodeRef = childAssocRef.getChildRef();
 
         // Keep track of new nodes to prevent firing of updates in the same transaction
@@ -156,7 +161,7 @@ public class CreateNodeRuleTrigger extends RuleTriggerAbstractBase
         }
         
         // Removes any rules that have already been triggered for that node
-        ruleService.removeRulePendingExecution(nodeRef);
+        runtimeRuleService.removeRulePendingExecution(nodeRef);
     }
     
     /**

source/java/org/alfresco/repo/rule/ruletrigger/OnContentUpdateRuleTrigger.java

@@ -88,6 +88,11 @@ public class OnContentUpdateRuleTrigger extends RuleTriggerAbstractBase
      */
     public void onContentUpdate(NodeRef nodeRef, boolean newContent)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         
         // Check the new content and make sure that we do indeed want to trigger the rule
         boolean fail = false;

source/java/org/alfresco/repo/rule/ruletrigger/OnCreateChildAssociationRuleTrigger.java

@@ -81,6 +81,12 @@ public class OnCreateChildAssociationRuleTrigger
 
     public void onCreateChildAssociation(ChildAssociationRef childAssocRef, boolean isNewNode)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
+
         // Avoid new nodes
         if (isNewNode)
         {

source/java/org/alfresco/repo/rule/ruletrigger/OnMoveNodeRuleTrigger.java

@@ -39,6 +39,11 @@ public class OnMoveNodeRuleTrigger extends RuleTriggerAbstractBase implements No
 
     public void onMoveNode(ChildAssociationRef oldChildAssocRef, ChildAssociationRef newChildAssocRef)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         // Check that it is not rename operation.
         if (!oldChildAssocRef.getParentRef().equals(newChildAssocRef.getParentRef()))
         {
@@ -48,6 +53,11 @@ public class OnMoveNodeRuleTrigger extends RuleTriggerAbstractBase implements No
 
     private void triggerChildrenRules(ChildAssociationRef oldChildAssocRef, ChildAssociationRef newChildAssocRef)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         triggerRules(newChildAssocRef.getParentRef(), newChildAssocRef.getChildRef());
         for (ChildAssociationRef ref : nodeService.getChildAssocs(newChildAssocRef.getChildRef()))
         {

source/java/org/alfresco/repo/rule/ruletrigger/OnPropertyUpdateRuleTrigger.java

@@ -125,6 +125,11 @@ public class OnPropertyUpdateRuleTrigger extends RuleTriggerAbstractBase
      */
     public void onUpdateProperties(NodeRef nodeRef, Map<QName, Serializable> before, Map<QName, Serializable> after)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         // Do not fire if the node has been created in this transaction
         Set<NodeRef> newNodeRefSet = TransactionalResourceHelper.getSet(RULE_TRIGGER_NEW_NODES);
         boolean wasCreatedInTxn = newNodeRefSet.contains(nodeRef);

source/java/org/alfresco/repo/rule/ruletrigger/RuleTriggerAbstractBase.java

@@ -30,6 +30,7 @@ import org.alfresco.service.cmr.dictionary.DictionaryService;
 import org.alfresco.service.cmr.repository.ContentService;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
+import org.alfresco.service.cmr.rule.RuleService;
 import org.alfresco.service.cmr.rule.RuleType;
 import org.alfresco.service.namespace.QName;
 
@@ -40,33 +41,34 @@ import org.alfresco.service.namespace.QName;
  */
 public abstract class RuleTriggerAbstractBase implements RuleTrigger
 {
+    /** the types (hardcoded) to ignore generally */
+    private static final Set<QName> IGNORE_TYPES;
+    /** the aspects (hardcoded) to ignore generally */
+    private static final Set<QName> IGNORE_ASPECTS;
+    static
+    {
+        IGNORE_TYPES = new HashSet<QName>(13);
+        IGNORE_TYPES.add(RuleModel.TYPE_RULE);
+        IGNORE_TYPES.add(ActionModel.TYPE_ACTION);
+        IGNORE_TYPES.add(ContentModel.TYPE_THUMBNAIL);
+        // Workaround to prevent rules running on cm:rating nodes (which happened for 'liked' folders ALF-8308 & ALF-8382)
+        IGNORE_TYPES.add(ContentModel.TYPE_RATING);
+
+        IGNORE_ASPECTS = new HashSet<QName>(13);
+        IGNORE_ASPECTS.add(ContentModel.ASPECT_TEMPORARY);
+    }
+    
     /**
      * A list of the rule types that are interested in this trigger
      */
     private Set<RuleType> ruleTypes = new HashSet<RuleType>();
 
-    /**
-     * The policy component
-     */
     protected PolicyComponent policyComponent;
-
-    /**
-     * The node service
-     */
     protected NodeService nodeService;
-    
-    /**
-     * The content service
-     */
     protected ContentService contentService;
-
-    /**
-     * The authentication Component
-     */
     protected AuthenticationComponent authenticationComponent;
-
-    /** The dictionary service */
     protected DictionaryService dictionaryService;
+    protected RuleService ruleService;
     
     /** 
      * Indicates whether the rule should be executed immediately or at the end of the transaction.
@@ -76,9 +78,6 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
     
     /**
      * Set the policy component
-     * 
-     * @param policyComponent
-     *            the policy component
      */
     public void setPolicyComponent(PolicyComponent policyComponent)
     {
@@ -87,9 +86,6 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
 
     /**
      * Set the node service
-     * 
-     * @param nodeService
-     *            the node service
      */
     public void setNodeService(NodeService nodeService)
     {
@@ -98,8 +94,6 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
     
     /**
      * Set the content service
-     * 
-     * @param contentService	the content service
      */
     public void setContentService(ContentService contentService) 
     {
@@ -116,8 +110,6 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
 
     /**
      * Set the dictionary service
-     * 
-     * @param dictionaryService     the dictionary service
      */
     public void setDictionaryService(DictionaryService dictionaryService)
     {
@@ -125,8 +117,15 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
     }
 
     /**
-     * Sets the values that indicates whether the rule should be executed immediately
+     * Set the RuleService to assist with enabled/disabled check
-     * or not.
+     */
+    public void setRuleService(RuleService ruleService)
+    {
+        this.ruleService = ruleService;
+    }
+
+    /**
+     * Sets the values that indicates whether the rule should be executed immediately or not.
      * 
      * @param executeRuleImmediately    true execute the rule immediaely, false otherwise
      */
@@ -148,13 +147,16 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
      * Trigger the rules that relate to any interested rule types for the node
      * references passed. 
      * 
-     * @param nodeRef
+     * @param nodeRef                   the node reference who rules are to be triggered
-     *            the node reference who rules are to be triggered
+     * @param actionedUponNodeRef       the node reference that will be actioned upon by the rules
-     * @param actionedUponNodeRef
-     *            the node reference that will be actioned upon by the rules
      */
     protected void triggerRules(NodeRef nodeRef, NodeRef actionedUponNodeRef)
     {
+        // Break out early if rules are off
+        if (!areRulesEnabled())
+        {
+            return;
+        }
     	// Do not trigger rules for rule and action type nodes
     	if (ignoreTrigger(actionedUponNodeRef) == false)
     	{
@@ -165,6 +167,16 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
     	}
     }
     
+    /**
+     * Helper method to allow triggers to check if rules are enabled or disabled
+     * (ALF-10839: Eliminate rule discovery overhead on property update when rules have been disabled)
+     * @return          <tt>true</tt> if rules are enabled
+     */
+    protected boolean areRulesEnabled()
+    {
+        return ruleService.isEnabled();
+    }
+    
     /**
      * Indicate whether the trigger should be ignored or not
      * @param actionedUponNodeRef	  actioned upon node reference
@@ -174,13 +186,17 @@ public abstract class RuleTriggerAbstractBase implements RuleTrigger
     {
     	boolean result = false;    	
     	QName typeQName = nodeService.getType(actionedUponNodeRef);
-    	if (typeQName.equals(RuleModel.TYPE_RULE) == true ||
+    	if (IGNORE_TYPES.contains(typeQName))
-    		typeQName.equals(ActionModel.TYPE_ACTION) == true ||
-    		typeQName.equals(ActionModel.TYPE_COMPOSITE_ACTION) == true ||
-    		typeQName.equals(ContentModel.TYPE_THUMBNAIL) == true)
     	{
     		result = true;
     	}
+    	for (QName aspectToIgnore : IGNORE_ASPECTS)
+        {
+            if (nodeService.hasAspect(actionedUponNodeRef, aspectToIgnore))
+            {
+                return true;
+            }
+        }
     	return result;
     }
 }

source/java/org/alfresco/repo/rule/ruletrigger/SingleNodeRefPolicyRuleTrigger.java

@@ -67,6 +67,11 @@ public class SingleNodeRefPolicyRuleTrigger extends RuleTriggerAbstractBase
 
     public void policyBehaviour(NodeRef nodeRef)
     {
+        // Break out early if rules are not enabled
+        if (!areRulesEnabled())
+        {
+            return;
+        }
         if (triggerParentRules == true)
         {
             List<ChildAssociationRef> parentsAssocRefs = this.nodeService.getParentAssocs(nodeRef);

source/java/org/alfresco/service/cmr/rule/RuleService.java

@@ -21,7 +21,6 @@ package org.alfresco.service.cmr.rule;
 import java.util.List;
 
 import org.alfresco.service.Auditable;
-import org.alfresco.service.PublicService;
 import org.alfresco.service.cmr.action.Action;
 import org.alfresco.service.cmr.repository.NodeRef;