Backporting fix for JENKINS-10424 from remoting project (jenkinsci/re…

…moting@4cd0ec9)
jenkinsci · Aug 17, 2011 · b130a59 · b130a59
1 parent 1351434
commit b130a59
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 4 deletions.
diff --git a/remoting/src/main/java/hudson/remoting/Channel.java b/remoting/src/main/java/hudson/remoting/Channel.java
@@ -146,7 +146,7 @@ public class Channel implements VirtualChannel, IChannel {
     /**
      * Objects exported via {@link #export(Class, Object)}.
      */
-    private final ExportTable<Object> exportedObjects = new ExportTable<Object>();
+    /*package (for test)*/ final ExportTable<Object> exportedObjects = new ExportTable<Object>();
 
     /**
      * {@link PipeWindow}s keyed by their OIDs (of the OutputStream exported by the other side.)
@@ -526,9 +526,10 @@ 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, 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());
+        // either local side will auto-unexport, or the remote side will unexport when it's GC-ed
+        boolean autoUnexportByCaller = exportedObjects.isRecording();
+        final int id = export(instance,autoUnexportByCaller);
+        return RemoteInvocationHandler.wrap(null, id, type, userProxy, autoUnexportByCaller);
     }
 
     /*package*/ int export(Object instance) {

diff --git a/remoting/src/main/java/hudson/remoting/ExportTable.java b/remoting/src/main/java/hudson/remoting/ExportTable.java
@@ -216,4 +216,8 @@ public synchronized void dump(PrintWriter w) throws IOException {
             e.allocationTrace.printStackTrace(w);
         }
     }
+
+    /*package*/ synchronized  boolean isExported(T o) {
+        return reverse.containsKey(o);
+    }
 }
diff --git a/remoting/src/test/java/hudson/remoting/ChannelTest.java b/remoting/src/test/java/hudson/remoting/ChannelTest.java
@@ -4,6 +4,7 @@
 
 import java.io.IOException;
 import java.io.ObjectInputStream;
+import java.io.Serializable;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -34,4 +35,89 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound
             throw new ClassCastException("foobar");
         }
     }
+
+    /**
+     * Objects exported during the request arg capturing is subject to caller auto-deallocation.
+     */
+    @Bug(10424)
+    public void testExportCallerDeallocation() throws Exception {
+        for (int i=0; i<100; i++) {
+            final GreeterImpl g = new GreeterImpl();
+            channel.call(new GreetingTask(g));
+            assertEquals(g.name,"Kohsuke");
+            assertTrue("in this scenario, auto-unexport by the caller should kick in.",
+                    !channel.exportedObjects.isExported(g));
+        }
+    }
+
+    /**
+     * Objects exported outside the request context should be deallocated by the callee.
+     */
+    @Bug(10424)
+    public void testExportCalleeDeallocation() throws Exception {
+        for (int j=0; j<10; j++) {
+            final GreeterImpl g = new GreeterImpl();
+            channel.call(new GreetingTask(channel.export(Greeter.class,g)));
+            assertEquals(g.name,"Kohsuke");
+            boolean isExported = channel.exportedObjects.isExported(g);
+            if (!isExported) {
+                // it is unlikely but possible that GC happens on remote node
+                // and 'g' gets unexported before we get to execute the above line
+                // if so, try again. if we kept failing after a number of retries,
+                // then it's highly suspicious that the caller is doing the deallocation,
+                // which is a bug.
+                continue;
+            }
+
+            // now we verify that 'g' gets eventually unexported by remote.
+            // to do so, we keep calling System.gc().
+            for (int i=0; i<30 && channel.exportedObjects.isExported(g); i++) {
+                channel.call(new GCTask());
+                Thread.sleep(100);
+            }
+
+            assertTrue(
+                    "Object isn't getting unexported by remote",
+                    !channel.exportedObjects.isExported(g));
+
+            return;
+        }
+
+        fail("in this scenario, remote will unexport this");
+    }
+
+    public interface Greeter {
+        void greet(String name);
+    }
+
+    private static class GreeterImpl implements Greeter, Serializable {
+        String name;
+        public void greet(String name) {
+            this.name = name;
+        }
+
+        private Object writeReplace() {
+            return Channel.current().export(Greeter.class,this);
+        }
+    }
+
+    private static class GreetingTask implements Callable<Object, IOException> {
+        private final Greeter g;
+
+        public GreetingTask(Greeter g) {
+            this.g = g;
+        }
+
+        public Object call() throws IOException {
+            g.greet("Kohsuke");
+            return null;
+        }
+    }
+
+    private static class GCTask implements Callable<Object, IOException> {
+        public Object call() throws IOException {
+            System.gc();
+            return null;
+        }
+    }
 }