[HUDSON-5048] Fixed the multi-classloader support in the remoting code. I...

[HUDSON-5048] Fixed the multi-classloader support in the remoting code. I confirmed that this fixes HUDSON-4993 without modifying the warnings/analysis-core plugin.

I'm letting Hudson run the whole test suite to see if there's any regression caused by this.

git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@24490 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/ClassicPluginStrategy.java

@@ -408,6 +408,20 @@ public class ClassicPluginStrategy implements PluginStrategy {
         }
 
         // TODO: delegate resources? watch out for diamond dependencies
+
+        @Override
+        protected URL findResource(String name) {
+            for (Dependency dep : dependencies) {
+                PluginWrapper p = pluginManager.getPlugin(dep.shortName);
+                if(p!=null) {
+                    URL url = p.classLoader.getResource(name);
+                    if (url!=null)
+                        return url;
+                }
+            }
+
+            return null;
+        }
     }
 
     /**

remoting/src/main/java/hudson/remoting/Capability.java

@@ -33,7 +33,7 @@ final class Capability implements Serializable {
     }
 
     Capability() {
-        this(0);
+        this(MASK_MULTI_CLASSLOADER);
     }
 
     /**
@@ -43,7 +43,7 @@ final class Capability implements Serializable {
      * @see MultiClassLoaderSerializer
      */
     boolean supportsMultiClassLoaderRPC() {
-        return (mask&1)!=0;
+        return (mask&MASK_MULTI_CLASSLOADER)!=0;
     }
 
     /**
@@ -70,6 +70,25 @@ final class Capability implements Serializable {
 
     private static final long serialVersionUID = 1L;
 
+    /**
+     * This was used briefly to indicate the use of {@link MultiClassLoaderSerializer}, but
+     * that was disabled (see HUDSON-4293) in Sep 2009. AFAIK no released version of Hudson
+     * exposed it, but since then the wire format of {@link MultiClassLoaderSerializer} has evolved
+     * in an incompatible way.
+     * <p>
+     * So just to be on the safe side, I assigned a different bit to indicate this feature {@link #MASK_MULTI_CLASSLOADER},
+     * so that even if there are remoting.jar out there that advertizes this bit, we won't be using
+     * the new {@link MultiClassLoaderSerializer} code.
+     * <p>
+     * If we ever use up all 64bits of long, we can probably come back and reuse this bit, as by then
+     * hopefully any such remoting.jar deployment is long gone. 
+     */
+    private static final long MASK_UNUSED1 = 1L;
+    /**
+     * Bit that indicates the use of {@link MultiClassLoaderSerializer}.
+     */
+    private static final long MASK_MULTI_CLASSLOADER = 2L;
+
     static final byte[] PREAMBLE;
 
     static {

remoting/src/main/java/hudson/remoting/MultiClassLoaderSerializer.java

@@ -18,6 +18,11 @@ import java.util.Map;
  * {@link ObjectInputStream}/{@link ObjectOutputStream} pair that can handle object graph that spans across
  * multiple classloaders.
  *
+ * <p>
+ * To pass around ClassLoaders, this class uses OID instead of {@link IClassLoader}, since doing so
+ * can results in recursive class resolution that may end up with NPE in ObjectInputStream.defaultReadFields like
+ * described in the comment from huybrechts in HUDSON-4293.
+ *
  * @author Kohsuke Kawaguchi
  * @see Capability#supportsMultiClassLoaderRPC() 
  */
@@ -38,16 +43,27 @@ class MultiClassLoaderSerializer {
         protected void annotateClass(Class<?> c) throws IOException {
             ClassLoader cl = c.getClassLoader();
             if (cl==null) {// bootstrap classloader. no need to export.
-                writeInt(-2);
+                writeInt(TAG_SYSTEMCLASSLOADER);
                 return;
             }
 
             Integer idx = classLoaders.get(cl);
             if (idx==null) {
                 classLoaders.put(cl,classLoaders.size());
-                writeInt(-1);
-                writeObject(RemoteClassLoader.export(cl,channel));
-            } else {
+                if (cl instanceof RemoteClassLoader) {
+                    int oid = ((RemoteClassLoader) cl).getOid(channel);
+                    if (oid>=0) {
+                        // this classloader came from where we are sending this classloader to.
+                        writeInt(TAG_LOCAL_CLASSLOADER);
+                        writeInt(oid);
+                        return;
+                    }
+                }
+
+                // tell the receiving side that they need to import a new classloader
+                writeInt(TAG_EXPORTED_CLASSLOADER);
+                writeInt(RemoteClassLoader.exportId(cl,channel));
+            } else {// reference to a classloader that's already written
                 writeInt(idx);
             }
         }
@@ -68,17 +84,20 @@ class MultiClassLoaderSerializer {
         }
 
         private ClassLoader readClassLoader() throws IOException, ClassNotFoundException {
+            ClassLoader cl;
             int code = readInt();
             switch (code) {
-            case -2:
+            case TAG_SYSTEMCLASSLOADER:
                 return null;
-            case -1:
-                // fill the entry with some value in preparation of recursive readObject below.
-                // this is actually only necessary for classLoader[0].
-                classLoaders.add(Channel.class.getClassLoader());
 
-                ClassLoader cl = channel.importedClassLoaders.get((IClassLoader) readObject());
-                classLoaders.set(classLoaders.size()-1,cl);
+            case TAG_LOCAL_CLASSLOADER:
+                cl = ((RemoteClassLoader.ClassLoaderProxy)channel.getExportedObject(readInt())).cl;
+                classLoaders.add(cl);
+                return cl;
+
+            case TAG_EXPORTED_CLASSLOADER:
+                cl = channel.importedClassLoaders.get(readInt());
+                classLoaders.add(cl);
                 return cl;
             default:
                 return classLoaders.get(code);
@@ -107,4 +126,19 @@ class MultiClassLoaderSerializer {
             return Proxy.getProxyClass(cl, classes);
         }
     }
+
+    /**
+     * Indicates that the class being sent should be loaded from the system classloader.
+     */
+    private static final int TAG_SYSTEMCLASSLOADER = -3;
+    /**
+     * Indicates that the class being sent originates from the sender side. The sender exports this classloader
+     * and sends its OID in the following int. The receiver will import this classloader to resolve the class.
+     */
+    private static final int TAG_EXPORTED_CLASSLOADER = -2;
+    /**
+     * Indicates that the class being sent originally came from the receiver. The following int indicates
+     * the OID of the classloader exported from the receiver, which the sender used.
+     */
+    private static final int TAG_LOCAL_CLASSLOADER = -1;
 }

remoting/src/main/java/hudson/remoting/RemoteClassLoader.java

@@ -89,6 +89,14 @@ final class RemoteClassLoader extends URLClassLoader {
         this.channel = RemoteInvocationHandler.unwrap(proxy);
     }
 
+    /**
+     * If this {@link RemoteClassLoader} represents a classloader from the specified channel,
+     * return its exported OID. Otherwise return -1.
+     */
+    /*package*/ int getOid(Channel channel) {
+        return RemoteInvocationHandler.unwrap(proxy,channel);
+    }
+
     protected Class<?> findClass(String name) throws ClassNotFoundException {
         try {
             // first attempt to load from locally fetched jars
@@ -97,17 +105,50 @@ final class RemoteClassLoader extends URLClassLoader {
             if(channel.isRestricted)
                 throw e;
             // delegate to remote
+            if (channel.remoteCapability.supportsMultiClassLoaderRPC()) {
+                /*
+                    In multi-classloader setup, RemoteClassLoaders do not retain the relationships among the original classloaders,
+                    so each RemoteClassLoader ends up loading classes on its own without delegating to other RemoteClassLoaders.
+
+                    See the classloader X/Y examples in HUDSON-5048 for the depiction of the problem.
+
+                    So instead, we find the right RemoteClassLoader to load the class on per class basis.
+                    The communication is optimized for the single classloader use, by always returning the class file image
+                    along with the reference to the initiating ClassLoader (if the initiating ClassLoader has already loaded this class,
+                    then the class file image is wasted.)
+                 */
+                long startTime = System.nanoTime();
+                ClassFile cf = proxy.fetch2(name);
+                channel.classLoadingTime.addAndGet(System.nanoTime()-startTime);
+                channel.classLoadingCount.incrementAndGet();
+
+                ClassLoader cl = channel.importedClassLoaders.get(cf.classLoader);
+                if (cl instanceof RemoteClassLoader) {
+                    RemoteClassLoader rcl = (RemoteClassLoader) cl;
+                    Class<?> c = rcl.findLoadedClass(name);
+                    if (c==null)
+                        c = rcl.loadClassFile(name,cf.classImage);
+                    return c;
+                } else {
+                    return cl.loadClass(name);
+                }
+            } else {
                 long startTime = System.nanoTime();
                 byte[] bytes = proxy.fetch(name);
                 channel.classLoadingTime.addAndGet(System.nanoTime()-startTime);
                 channel.classLoadingCount.incrementAndGet();
 
+                return loadClassFile(name, bytes);
+            }
+        }
+    }
+
+    private Class<?> loadClassFile(String name, byte[] bytes) {
         // define package
         definePackage(name);
 
         return defineClass(name, bytes, 0, bytes.length);
     }
-    }
 
     /**
      * Defining a package is necessary to make {@link Class#getPackage()} work,
@@ -232,12 +273,28 @@ final class RemoteClassLoader extends URLClassLoader {
         }
     }
 
+    static class ClassFile implements Serializable {
+        /**
+         * oid of the classloader that should load this class.
+         */
+        final int classLoader;
+        final byte[] classImage;
+
+        ClassFile(int classLoader, byte[] classImage) {
+            this.classLoader = classLoader;
+            this.classImage = classImage;
+        }
+
+        private static final long serialVersionUID = 1L;
+    }
+
     /**
      * Remoting interface.
      */
     /*package*/ static interface IClassLoader {
         byte[] fetchJar(URL url) throws IOException;
         byte[] fetch(String className) throws ClassNotFoundException;
+        ClassFile fetch2(String className) throws ClassNotFoundException;
         byte[] getResource(String name) throws IOException;
         byte[][] getResources(String name) throws IOException;
     }
@@ -251,21 +308,23 @@ final class RemoteClassLoader extends URLClassLoader {
                 return new RemoteIClassLoader(oid,rcl.proxy);
             }
         }
-        return local.export(IClassLoader.class, new ClassLoaderProxy(cl), false);
+        return local.export(IClassLoader.class, new ClassLoaderProxy(cl,local), false);
     }
 
     /**
      * Exports and just returns the object ID, instead of obtaining the proxy.
      */
     static int exportId(ClassLoader cl, Channel local) {
-        return local.export(new ClassLoaderProxy(cl));
+        return local.export(new ClassLoaderProxy(cl,local), false);
     }
 
     /*package*/ static final class ClassLoaderProxy implements IClassLoader {
-        private final ClassLoader cl;
+        final ClassLoader cl;
+        final Channel channel;
 
-        public ClassLoaderProxy(ClassLoader cl) {
+        public ClassLoaderProxy(ClassLoader cl, Channel channel) {
             this.cl = cl;
+            this.channel = channel;
         }
 
         public byte[] fetchJar(URL url) throws IOException {
@@ -284,6 +343,17 @@ final class RemoteClassLoader extends URLClassLoader {
             }
         }
 
+        public ClassFile fetch2(String className) throws ClassNotFoundException {
+            ClassLoader ecl = cl.loadClass(className).getClassLoader();
+
+            try {
+                return new ClassFile(
+                        exportId(ecl,channel),
+                        readFully(ecl.getResourceAsStream(className.replace('.', '/') + ".class")));
+            } catch (IOException e) {
+                throw new ClassNotFoundException();
+            }
+        }
 
         public byte[] getResource(String name) throws IOException {
             InputStream in = cl.getResourceAsStream(name);
@@ -352,6 +422,10 @@ final class RemoteClassLoader extends URLClassLoader {
             return proxy.fetch(className);
         }
 
+        public ClassFile fetch2(String className) throws ClassNotFoundException {
+            return proxy.fetch2(className);
+        }
+
         public byte[] getResource(String name) throws IOException {
             return proxy.getResource(name);
         }

remoting/src/test/java/hudson/remoting/ChannelTest.java

@@ -5,7 +5,6 @@ package hudson.remoting;
  */
 public class ChannelTest extends RmiTestBase {
     public void testCapability() {
-        // for now this is disabled
-        assertTrue(!channel.remoteCapability.supportsMultiClassLoaderRPC());
+        assertTrue(channel.remoteCapability.supportsMultiClassLoaderRPC());
     }
 }