Manage potential race to inject same rule from concurrent threads -- …

…fixes Byteman-390

agent/src/main/java/org/jboss/byteman/agent/Retransformer.java

@@ -182,7 +182,7 @@ public void installScript(List<String> scriptTexts, List<String> scriptNames, Pr
                     if(newTransformSet == null || newTransformSet.isInstalled()) {
                         if(oldTransformSet.isInstalled()) {
                             // we need to run an uninstall for the old transform set
-                            oldTransformSet.getInstalledRule().uninstalled();
+                            oldTransformSet.getRule().uninstalled();
                         }
                     } else {
                         // copy across the rule used for the prior
@@ -191,9 +191,9 @@ public void installScript(List<String> scriptTexts, List<String> scriptNames, Pr
                         // if any of the newly injected rules pass
                         // ensureTypeCheckCompiled
                         if(newTransformSet != null) {
-                            newTransformSet.setInstalled(oldTransformSet.getInstalledRule());
+                            newTransformSet.setInstalled(oldTransformSet.getRule());
                         } else {
-                            newRuleScript.ensureTransformSet(oldTransformSet.getLoader(), oldTransformSet.getTriggerClass(), oldTransformSet.getInstalledRule());
+                            newRuleScript.ensureTransformSet(oldTransformSet.getLoader(), oldTransformSet.getTriggerClass(), oldTransformSet.getRule());
                         }
                     }
                 }
@@ -345,6 +345,9 @@ public void removeScripts(List<String> scriptTexts, PrintWriter out) throws Exce
         // the rule key
 
         for (RuleScript oldRuleScript : toBeRemoved) {
+            // mark the script as deleted so it doesn't get run any more
+            oldRuleScript.setDeleted();
+            // now deal with uninstall, allowing for possible reinstall
             RuleScript newRuleScript = scriptRepository.scriptForRuleName(oldRuleScript.getName());
             // new script may not exist!
             if (newRuleScript != null) {
@@ -354,33 +357,33 @@ public void removeScripts(List<String> scriptTexts, PrintWriter out) throws Exce
                         TransformSet newTransformSet = newRuleScript.lookupTransformSet(oldTransformSet.getLoader(), oldTransformSet.getTriggerClass());
                         if(newTransformSet == null || newTransformSet.isInstalled()) {
                             if(oldTransformSet.isInstalled()) {
-                                // we need to run an uninstall for the old transform set
-                                oldTransformSet.getInstalledRule().uninstalled();
+                                // new rule has already been installed so we need
+                                // to run an uninstall for the old transform set
+                                oldTransformSet.getRule().uninstalled();
                             }
                         } else {
-                            // copy across the rule used for the prior
-                            // install so we can use it for a later uninstall
-                            // it will be replaced with a new instance
-                            // if any of the newly injected rules pass
-                            // ensureTypeCheckCompiled
-                            if(newTransformSet != null) {
-                                newTransformSet.setInstalled(oldTransformSet.getInstalledRule());
-                            } else {
-                                newRuleScript.ensureTransformSet(oldTransformSet.getLoader(), oldTransformSet.getTriggerClass(), oldTransformSet.getInstalledRule());
-                            }
+                            // new rule is not yet installed so we can elide
+                            // the uninstalled + installed lifecycle events
+                            // we have to copy across the rule used for the prior
+                            // install so we can use it as the default argument
+                            // for a later uninstall. it will be replaced with a
+                            // new instance if any of the newly injected rules
+                            // pass ensureTypeCheckCompiled
+                            newTransformSet.setInstalled(oldTransformSet.getRule());
                         }
                     }
                 }
             } else {
                 for (TransformSet oldTransformSet : oldRuleScript.getTransformSets()) {
                     if(oldTransformSet.isInstalled()) {
                         // we need to run an uninstall for the old transform set
-                        oldTransformSet.getInstalledRule().uninstalled();
+                        oldTransformSet.getRule().uninstalled();
                     }
                 }
+                out.println("uninstall RULE " + oldRuleScript.getName());
             }
-            out.println("uninstall RULE " + oldRuleScript.getName());
         }
+        // now purge the rules for the old script
     }
 
     public void appendJarFile(PrintWriter out, JarFile jarfile, boolean isBoot) throws Exception

agent/src/main/java/org/jboss/byteman/agent/RuleScript.java

@@ -27,6 +27,7 @@
 import org.jboss.byteman.rule.type.TypeHelper;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.io.StringWriter;
 import java.io.PrintWriter;
@@ -39,6 +40,20 @@
 
 public class RuleScript
 {
+    /**
+     * a counter used to ensure rule identifiers are unique
+     */
+    private static int nextId = 0;
+
+    /**
+     * a method to return the next available counter for use in constructing a key for a rule
+     * @return the next id
+     */
+    private synchronized static int nextId()
+    {
+        return nextId++;
+    }
+
     /**
      * the name of the rule from which this script is derived
      */
@@ -99,6 +114,24 @@ public class RuleScript
      * true if this rule should be compiled to bytecode otherwise false
      */
     private final boolean compileToBytecode;
+    /**
+     * hash map used to lookup a key used at injection time to identify a
+     * rule cloned from this script for injection into a specific trigger
+     * method. the map translates a string constructed from the trigger class
+     * name, method name, method descriptor and class loader hash to a unique
+     * key based on the rule name. This ensures that concurrent attempts to inject
+     * the rule into the same trigger method will employ the same key and hence
+     * perform exactly the same transformation. That way it does not matter which
+     * of the transformations are accepted or dropped by the JVM when defining a
+     * newly loaded class. Any transform result for a given key is as valid as
+     * any other.
+     */
+    private final HashMap<String, String> keySet;
+    /**
+     * base string from which to construct rule injection keys
+     */
+    private final String key_base;
+
     /**
      * a list of records identifying transforms associated with a specific class.
      * each set is identified by the name of a trigger class and the class's
@@ -146,6 +179,8 @@ public RuleScript(String name, String targetClass, boolean isInterface, boolean
         this.file = file;
         this.compileToBytecode = compileToBytecode;
         this.transformSets = new ArrayList<TransformSet>();
+        this.keySet = new HashMap<String, String>();
+        this.key_base = name + "_" + nextId();
     }
 
     public String getName() {
@@ -196,6 +231,20 @@ public String getFile()
 
     public boolean isCompileToBytecode() { return compileToBytecode; }
 
+    public synchronized String getRuleKey(String triggerClassName, String triggerMethodName, String triggerMethodDescriptor, ClassLoader loader) {
+        if (triggerMethodName == null) {
+            // this can happen when we get errors ???
+            return key_base;
+        }
+        String lookup =  triggerClassName + "." + triggerMethodName + TypeHelper.internalizeDescriptor(triggerMethodDescriptor) + "_" + loader.hashCode();
+        String result = keySet.get(lookup);
+        if (result == null) {
+            result = key_base + ":" + keySet.size();
+            keySet.put(lookup, result);
+        }
+        return result;
+    }
+
     /**
      * getter for list of transforms applied for this script. must be called synchronized on the script.
      * @return the list of transforms
@@ -260,23 +309,8 @@ public synchronized boolean recordFailedTransform(ClassLoader loader, String int
     }
 
     /**
-     * record the fact that a trigger call has been successfully installed into bytecode associated with a specific
-     * class and loader and a corresponding rule instance been installed
-     * @param loader the loader of the class for which injection was attempted
-     * @param internalClassName the internal Java name of the class
-     * @param triggerMethodName the name of the method injected into
-     * @param desc the descriptor of the method injected into
-     * @param rule the rule which was injected
-     * @return true if the successful injection was recorded false if not
-     */
-    public synchronized boolean recordMethodTransform(ClassLoader loader, String internalClassName, String triggerMethodName, String desc, Rule  rule)
-    {
-        return recordTransform(loader, internalClassName, triggerMethodName, desc, rule, null);
-    }
-
-    /**
-     * record the fact that a trigger call has failed to install into bytecode associated with a specific
-     * class and loader
+     * record the fact that a trigger call has succeeded or else failed to install into bytecode
+     * associated with a specific class and loader
      * @param loader the loader of the class for which injection was attempted
      * @param internalClassName the internal Java name of the class
      * @param triggerMethodName the name of the method injected into
@@ -345,7 +379,9 @@ public synchronized boolean recordCompile(Rule rule, String triggerClass, ClassL
         // find an existing transform set or create a new one
         TransformSet transformSet = ensureTransformSet(loader, triggerClass, null);
         for (Transform transform : transformSet.getTransforms()) {
-            if(transform.getRule() == rule) {
+            // transform may not employ the same rule
+            // but it may have the same key.
+            if(transform.getRule().getKey() == rule.getKey()) {
                 transform.setCompiled(successful, detail);
                 boolean isInstalled = transformSet.isInstalled();
                 // record this as the latest rule to be installed

agent/src/main/java/org/jboss/byteman/agent/Transform.java

@@ -33,7 +33,13 @@
 public class Transform
 {
     private ClassLoader loader;
+    /**
+     * fully qualified internal name of class
+     */
     private String internalClassName;
+    /**
+     * full method name including descriptor
+     */
     private String triggerMethodName;
     private Rule rule;
     private Throwable throwable;
@@ -42,10 +48,6 @@ public class Transform
     private boolean successful;
     private String detail;
 
-    public Transform(ClassLoader loader, String internalClassName, Rule rule) {
-        this(loader, internalClassName, null, rule, null);
-    }
-
     public Transform(ClassLoader loader, String internalClassName, String triggerMethodName, Rule rule, Throwable th) {
         this.loader = loader;
         this.internalClassName = internalClassName;

agent/src/main/java/org/jboss/byteman/agent/TransformContext.java

@@ -82,7 +82,7 @@ public byte[] transform(byte[] targetClassBytes)
         // associated with this rule's script
         // which specify the same class and loader
 
-        ruleScript.purge(loader, triggerClassName);
+        // ruleScript.purge(loader, triggerClassName);
 
         try {
             parseRule();
@@ -332,7 +332,7 @@ private boolean notifyRules()
         if (failed) {
             // we had an injection failure so purge all successfully
             // injected rules
-            purgeRules();
+            // purgeRules();
 
             return false;
         } else {
@@ -343,7 +343,7 @@ private boolean notifyRules()
                 Rule rule = ruleMap.get(key);
                 if(!ruleScript.recordTransform(loader, triggerClassName, triggerMethodName, triggerMethodDescriptor, rule, null)) {
                     // rule script must have been deleted so purge rules and avoid installing the transformed code
-                    purgeRules();
+                    // purgeRules();
 
                     return false;
                 }

agent/src/main/java/org/jboss/byteman/agent/TransformSet.java

@@ -25,30 +25,41 @@
 package org.jboss.byteman.agent;
 
 import org.jboss.byteman.rule.Rule;
+import org.jboss.byteman.rule.helper.Helper;
 
 import java.util.ArrayList;
 import java.util.List;
 /**
  * A TransformSet groups together a set of Transform records which
- * share a common classloader and trigger class name. This grouping
- * ensures that all transforms arising from a specific retransform
- * operation can be managed as a unit. In particular this is needed
- * in order to allow installation and uninstallation of a rule to
- * be performed consistently.
+ * share a common classloader, trigger class name (and RuleScript).
+ * The set includes details of successful or failed transforms.
+ *
+ * This grouping ensures that all transforms arising from a specific
+ * retransform operation for a new, modified or deleted script can
+ * be managed as a unit. In particular this is needed in order to
+ * allow installation and uninstallation of a rule to be performed
+ * consistently.
+ *
+ * Note that although the loader and trigger class name uniquely
+ * identify a single trigger class a transform set may still
+ * contain more than one successful transform. That is possible
+ * because the RuleScript may omit a descriptor and hence may match
+ * multiple overloaded variants of the method named in the rule's
+ * METHOD clause.
  */
 public class TransformSet
 {
     private ClassLoader loader;
     private String triggerClass;
     private List<Transform> transforms;
-    private Rule installedRule;
+    private Rule rule;
 
     public TransformSet(ClassLoader loader, String triggerClass)
     {
         this.loader = loader;
         this.triggerClass = triggerClass;
         this.transforms = new ArrayList<Transform>();
-        this.installedRule = null;
+        this.rule = null;
     }
 
     public boolean isFor(ClassLoader loader, String triggerClass)
@@ -58,7 +69,25 @@ public boolean isFor(ClassLoader loader, String triggerClass)
 
     public void add(Transform transform)
     {
+        if (transform.getThrowable() == null) {
+            // transform was successful. see if we have another
+            // transform for the same method and if so just stick
+            // with the existing one as both are equivalent
+            String key = transform.getRule().getKey();
+            for (Transform current : transforms) {
+                if (current.getRule() != null && key.equals(current.getRule().getKey())) {
+                    // the other transform should not have resulted in an error
+                    if (transform.getThrowable() != null && Transformer.isVerbose()) {
+                        Helper.verbose("TransformSet.add : mismatch between successful and failed transforms with key " + key);
+                        Helper.verboseTraceException(transform.getThrowable());
+                    }
+                    return;
+                }
+            }
+        }
+        // add the new transform to the list
         transforms.add(transform);
+
     }
 
     public ClassLoader getLoader()
@@ -75,17 +104,17 @@ public String getTriggerClass() {
      */
     public boolean isInstalled()
     {
-        return getInstalledRule() != null;
+        return getRule() != null;
     }
 
-    public void setInstalled(Rule rule)
+    public void setInstalled(Rule key)
     {
-        installedRule = rule;
+        this.rule = key;
     }
 
-    public Rule getInstalledRule()
+    public Rule getRule()
     {
-        return installedRule;
+        return rule;
     }
 
     public List<Transform> getTransforms()