svn commit: r727234 - in /geronimo/server/trunk/framework/modules: geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/ geronimo-transformer/ geronimo-transformer/src/main/java/org/apache/geronimo/transformer/

[ke...@apache.org]

Author: kevan
Date: Tue Dec 16 16:36:29 2008
New Revision: 727234

URL: http://svn.apache.org/viewvc?rev=727234&view=rev
Log:
GERONIMO-4458 Fix ClassLoader deadlocks caused by ClassFileTransformer processing. Transformation could cause cycles in our classloader DAG. New checking prevents cycles.

Modified:
    geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/MultiParentClassLoader.java
    geronimo/server/trunk/framework/modules/geronimo-transformer/pom.xml
    geronimo/server/trunk/framework/modules/geronimo-transformer/src/main/java/org/apache/geronimo/transformer/TransformerCollection.java

Modified: geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/MultiParentClassLoader.java
URL: http://svn.apache.org/viewvc/geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/MultiParentClassLoader.java?rev=727234&r1=727233&r2=727234&view=diff
==============================================================================
--- geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/MultiParentClassLoader.java (original)
+++ geronimo/server/trunk/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/MultiParentClassLoader.java Tue Dec 16 16:36:29 2008
@@ -606,6 +606,31 @@
         return hiddenRule.isFilteredResource(name);
     }
 
+    public static Set<ClassLoader> getAncestors(ClassLoader cl) {
+        Set<ClassLoader> ancestors = new HashSet();
+        getAncestorsInternal(ancestors, cl);
+        return ancestors;
+    }
+
+    /**
+     * Recursive method to calculate all ClassLoader parents of a given ClassLoader.
+     */
+    private static void getAncestorsInternal(Set<ClassLoader> ancestors, ClassLoader childClassLoader) {
+        if (childClassLoader == null) {
+            return;
+        }
+        if (childClassLoader instanceof MultiParentClassLoader) {
+            for (ClassLoader cl: ((MultiParentClassLoader)childClassLoader).parents) {
+                ancestors.add(cl);
+                getAncestorsInternal(ancestors, cl);
+            }
+        } else {
+            ClassLoader cl = childClassLoader.getParent();
+            ancestors.add(cl);
+            getAncestorsInternal(ancestors, cl);
+        }
+    }
+    
     public String toString() {
         StringBuilder b = new StringBuilder();
         if (LONG_CLASSLOADER_TO_STRING) {

Modified: geronimo/server/trunk/framework/modules/geronimo-transformer/pom.xml
URL: http://svn.apache.org/viewvc/geronimo/server/trunk/framework/modules/geronimo-transformer/pom.xml?rev=727234&r1=727233&r2=727234&view=diff
==============================================================================
--- geronimo/server/trunk/framework/modules/geronimo-transformer/pom.xml (original)
+++ geronimo/server/trunk/framework/modules/geronimo-transformer/pom.xml Tue Dec 16 16:36:29 2008
@@ -31,5 +31,13 @@
     <artifactId>geronimo-transformer</artifactId>
     <name>Geronimo Framework, Modules :: Transformer</name>
 
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.geronimo.framework</groupId>
+            <artifactId>geronimo-kernel</artifactId>
+            <version>${version}</version>
+        </dependency>
+    </dependencies>
+
 </project>
 

Modified: geronimo/server/trunk/framework/modules/geronimo-transformer/src/main/java/org/apache/geronimo/transformer/TransformerCollection.java
URL: http://svn.apache.org/viewvc/geronimo/server/trunk/framework/modules/geronimo-transformer/src/main/java/org/apache/geronimo/transformer/TransformerCollection.java?rev=727234&r1=727233&r2=727234&view=diff
==============================================================================
--- geronimo/server/trunk/framework/modules/geronimo-transformer/src/main/java/org/apache/geronimo/transformer/TransformerCollection.java (original)
+++ geronimo/server/trunk/framework/modules/geronimo-transformer/src/main/java/org/apache/geronimo/transformer/TransformerCollection.java Tue Dec 16 16:36:29 2008
@@ -16,44 +16,86 @@
  */
 package org.apache.geronimo.transformer;
 
+import java.lang.ClassLoader;
 import java.lang.instrument.ClassFileTransformer;
 import java.lang.instrument.IllegalClassFormatException;
 import java.security.ProtectionDomain;
 import java.util.List;
 import java.util.ArrayList;
 import java.util.Iterator;
+import java.util.Set;
+
+import org.apache.geronimo.kernel.config.MultiParentClassLoader;
 
 /**
+ * TransformerCollection is the control point for all ClassFileTransformations within the a Geronimo server.
+ * TransformerCollection will be the only ClassFileTransformer registered to Java. Whenever a new Class may
+ * require transformation, the transform() method will be invoked. transform() will in turn invoke each of the 
+ * ClassFileTransformers which have been configured.
+ * 
+ * One exception: if the ClassLoader of the Class to be transformed is a parent ClassLoader of the 
+ * TransformerCollection class, the transform will be ignored. Otherwise, we'll introduce a cycle in the ClassLoader
+ * DAG, and ClassLoader deadlocks could occur. 
+ *
  * @version $Rev$ $Date$
  */
 public class TransformerCollection implements ClassFileTransformer {
 
-    private final List<ClassFileTransformer> transformers = new ArrayList<ClassFileTransformer>();
-
+    private final List<TransformerWrapper> transformers = new ArrayList<TransformerWrapper>();
+    
     // hack to force load of ArrayList$Itr class. This avoids a potential Classloader deadlock during startup
     // see GERONIMO-3687
     {
         for (ClassFileTransformer transformer : transformers) {
         }
     }
-
+    
     public byte[] transform(ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException {
+        
         boolean changed = false;
         for (ClassFileTransformer transformer : transformers) {
             byte[] transformed = transformer.transform(loader, className, classBeingRedefined, protectionDomain, classfileBuffer);
             if (transformed != null) {
                 changed = true;
                 classfileBuffer = transformed;
-            }
+            }       
         }
         return changed? classfileBuffer: null;
     }
 
     public void addTransformer(ClassFileTransformer classFileTransformer) {
-        transformers.add(classFileTransformer);
+        transformers.add(new TransformerWrapper(classFileTransformer));
     }
 
     public void removeTransformer(ClassFileTransformer classFileTransformer) {
-        transformers.remove(classFileTransformer);
+        for (TransformerWrapper wrapper : transformers) {
+            if (wrapper.delegate == classFileTransformer) {
+                transformers.remove(wrapper);
+                return;
+            }
+        }
     }
+
+    /**
+     * Private wrapper class for ClassFileTransformers. Purpose of the wrapper class is to avoid potential ClassLoader deadlocks.
+     */
+    private class TransformerWrapper implements ClassFileTransformer {
+
+        private final ClassFileTransformer delegate;
+        private final Set<ClassLoader> ancestorClassLoaders;
+
+        TransformerWrapper(ClassFileTransformer transformer) {
+            this.delegate = transformer;
+            this.ancestorClassLoaders = MultiParentClassLoader.getAncestors(delegate.getClass().getClassLoader());
+        }
+
+        public byte[] transform(ClassLoader loader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException {
+            // if the loader is a parent of the delegate ClassFileTransformer, ignore this transform. This avoids potential ClassLoader deadlocks
+            if (ancestorClassLoaders.contains(loader)) {
+                return null;
+            }
+            return delegate.transform(loader, className, classBeingRedefined, protectionDomain, classfileBuffer);
+        }
+    }
+
 }