[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. (cherry picked from commit 74e35d8)
jenkinsci · May 23, 2011 · afcb555 · afcb555
1 parent 8511294
commit afcb555
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 6 deletions.
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.
      */