[FIXED JENKINS-9017]

Fixed incorrect de-allocation of a classloader from the exported object table. The fix is a defense-in-depth; it prevents classloaders referenced in the object graph from doubly released, then we also make it impossible for bugs like this to deallocate the key classloader.
jenkinsci · May 11, 2011 · 74e35d8 · 74e35d8
1 parent ac28899
commit 74e35d8
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 6 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,6 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    Fixed a <tt>ClassCastException</tt> caused by multiple loading of the same class in different classloaders.
+    (<a href="http://issues.jenkins-ci.org/browse/JENKINS-9017">issue 9017</a>)
   <li class=rfe>
     Gracefully handle old slave.jar to avoid <tt>AbstractMethodError</tt>
     (<a href="https://groups.google.com/d/topic/jenkinsci-dev/KqFw4nfiQdE/discussion">thread</a>)

diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java
@@ -342,6 +342,11 @@ public void onClosed(Channel c, IOException cause) {
             log.println("WARNING: "+remoteFs+" looks suspiciously like Windows path. Maybe you meant "+remoteFs.replace('\\','/')+"?");
         FilePath root = new FilePath(channel,getNode().getRemoteFS());
 
+        // reference counting problem is known to happen, such as JENKINS-9017, and so as a preventive measure
+        // we pin the base classloader so that it'll never get GCed. When this classloader gets released,
+        // it'll have a catastrophic impact on the communication.
+        channel.pinClassLoader(getClass().getClassLoader());
+
         channel.call(new SlaveInitializer());
         channel.call(new WindowsSlaveInstaller(remoteFs));
         for (ComputerListener cl : ComputerListener.all())

diff --git a/remoting/src/main/java/hudson/remoting/Channel.java b/remoting/src/main/java/hudson/remoting/Channel.java
@@ -526,9 +526,9 @@ public <T> T export(Class<T> type, T instance) {
                 logger.log(Level.WARNING, "Unable to send GC command",e);
             }
 
-        // proxy will unexport this instance when it's GC-ed on the remote machine.
-        final int id = export(instance);
-        return RemoteInvocationHandler.wrap(null,id,type,userProxy,exportedObjects.isRecording());
+        // proxy will unexport this instance when it's GC-ed on the remote machine, so don't unexport on our side automatically at the end of a call.
+        final int id = export(instance,false);
+        return RemoteInvocationHandler.wrap(null, id, type, userProxy, exportedObjects.isRecording());
     }
 
     /*package*/ int export(Object instance) {
@@ -547,6 +547,22 @@ public <T> T export(Class<T> type, T instance) {
         exportedObjects.unexportByOid(id);
     }
 
+    /**
+     * Increase reference count so much to effectively prevent de-allocation.
+     *
+     * @see ExportTable.Entry#pin()
+     */
+    public void pin(Object instance) {
+        exportedObjects.pin(instance);
+    }
+
+    /**
+     * {@linkplain #pin(Object) Pin down} the exported classloader.
+     */
+    public void pinClassLoader(ClassLoader cl) {
+        RemoteClassLoader.pin(cl,this);
+    }
+
     /**
      * Preloads jar files on the remote side.
      *

diff --git a/remoting/src/main/java/hudson/remoting/ExportTable.java b/remoting/src/main/java/hudson/remoting/ExportTable.java
@@ -67,7 +67,6 @@ private final class Entry {
             // force the computation of the stack trace in a Java friendly data structure,
             // so that the call stack can be seen from the heap dump after the fact. 
             allocationTrace.getStackTrace();
-            addRef();
 
             table.put(id,this);
             reverse.put(object,this);
@@ -77,6 +76,16 @@ void addRef() {
             referenceCount++;
         }
 
+        /**
+         * Increase reference count so much to effectively prevent de-allocation.
+         * If the reference counting is correct, we just need to increment by one,
+         * but this makes it safer even in case of some reference counting errors
+         * (and we can still detect the problem by comparing the reference count with the magic value.
+         */
+        void pin() {
+            referenceCount += Integer.MAX_VALUE/2;
+        }
+
         void release() {
             if(--referenceCount==0) {
                 table.remove(id);
@@ -155,8 +164,7 @@ public synchronized int export(T t, boolean notifyListener) {
         Entry e = reverse.get(t);
         if(e==null)
             e = new Entry(t);
-        else
-            e.addRef();
+        e.addRef();
 
         if(notifyListener) {
             ExportList l = lists.get();
@@ -166,6 +174,13 @@ public synchronized int export(T t, boolean notifyListener) {
         return e.id;
     }
 
+    /*package*/ synchronized void pin(T t) {
+        Entry e = reverse.get(t);
+        if(e==null)
+            e = new Entry(t);
+        e.pin();
+    }
+
     public synchronized T get(int id) {
         Entry e = table.get(id);
         if(e!=null) return e.object;

diff --git a/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java b/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java
@@ -352,6 +352,16 @@ public static IClassLoader export(ClassLoader cl, Channel local) {
         return local.export(IClassLoader.class, new ClassLoaderProxy(cl,local), false);
     }
 
+    public static void pin(ClassLoader cl, Channel local) {
+        if (cl instanceof RemoteClassLoader) {
+            // check if this is a remote classloader from the channel
+            final RemoteClassLoader rcl = (RemoteClassLoader) cl;
+            int oid = RemoteInvocationHandler.unwrap(rcl.proxy, local);
+            if(oid!=-1) return;
+        }
+        local.pin(new ClassLoaderProxy(cl,local));
+    }
+
     /**
      * Exports and just returns the object ID, instead of obtaining the proxy.
      */