+ * 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. + *
+ * 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 { diff --git a/remoting/src/main/java/hudson/remoting/MultiClassLoaderSerializer.java b/remoting/src/main/java/hudson/remoting/MultiClassLoaderSerializer.java index 2b951fb7798f60824c71e7f15f6d059d3dcbf2bd..55b1c3f60cce7424bbf82a3d1ed734fc577c816b 100644 --- a/remoting/src/main/java/hudson/remoting/MultiClassLoaderSerializer.java +++ b/remoting/src/main/java/hudson/remoting/MultiClassLoaderSerializer.java @@ -16,7 +16,12 @@ import java.util.Map; /** * {@link ObjectInputStream}/{@link ObjectOutputStream} pair that can handle object graph that spans across - * multiple classloaders. + * multiple classloaders. + * + *
+ * 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; } diff --git a/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java b/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java index 0064c25b3f7f9e3cf1a3372aac5d9b87ba0c5f59..f2970c839424e2de3b892ad49470dffd25549fc8 100644 --- a/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java +++ b/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,16 +105,49 @@ final class RemoteClassLoader extends URLClassLoader { if(channel.isRestricted) throw e; // delegate to remote - long startTime = System.nanoTime(); - byte[] bytes = proxy.fetch(name); - channel.classLoadingTime.addAndGet(System.nanoTime()-startTime); - channel.classLoadingCount.incrementAndGet(); + 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); + } + } + } - // define package - definePackage(name); + private Class loadClassFile(String name, byte[] bytes) { + // define package + definePackage(name); - return defineClass(name, bytes, 0, bytes.length); - } + return defineClass(name, bytes, 0, bytes.length); } /** @@ -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); } diff --git a/remoting/src/test/java/hudson/remoting/ChannelTest.java b/remoting/src/test/java/hudson/remoting/ChannelTest.java index d0250a62cbf3f13d32c57dc3cb5ae956913d39ca..8b5b2f9fa3f0bb76de20e970cd128574fc676629 100644 --- a/remoting/src/test/java/hudson/remoting/ChannelTest.java +++ b/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()); } }