Merged #52.

CHANGES.md

@@ -9,6 +9,7 @@ Only noting significant user-visible or major API changes, not internal code cle
 * Fixing `java.io.NotSerializableException: org.jenkinsci.plugins.workflow.support.steps.StageStepExecution$CanceledCause` thrown from certain scripts using `stage`.
 * JENKINS-26605: Missing link to _Full Log_ under _Running Steps_ when a single step produced >150Kb of output.
 * `catchError` was incorrectly setting build status to failed when it was merely aborted, canceled, etc.
+* PR 52: fixed some memory leaks causing the permanent generation and heap to grow unbounded after many flow builds.
 
 ## 1.2 (Jan 24 2015)
 

aggregator/src/test/java/org/jenkinsci/plugins/workflow/CpsFlowExecutionTest.java

@@ -0,0 +1,98 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2015 Jesse Glick.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.workflow;
+
+import java.lang.ref.WeakReference;
+import java.lang.reflect.Field;
+import org.codehaus.groovy.transform.ASTTransformationVisitor;
+import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MemoryAssert;
+
+public class CpsFlowExecutionTest {
+
+    @Rule public JenkinsRule r = new JenkinsRule();
+
+    private static WeakReference<ClassLoader> LOADER;
+    public static void register(Object o) {
+        LOADER = new WeakReference<ClassLoader>(o.getClass().getClassLoader());
+    }
+    @Test public void loaderReleased() throws Exception {
+        WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition(CpsFlowExecutionTest.class.getName() + ".register(this)"));
+        r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertNotNull(LOADER);
+        System.err.println(LOADER.get());
+        {
+            // TODO in Groovy 1.8.9 this keeps static state, but only for the last script (as also noted in JENKINS-23762).
+            // The fix of GROOVY-5025 (62bfb68) in 1.9 addresses this, which we would get if JENKINS-21249 is implemented.
+            Field f = ASTTransformationVisitor.class.getDeclaredField("compUnit");
+            f.setAccessible(true);
+            f.set(null, null);
+        }
+        MemoryAssert.assertGC(LOADER);
+    }
+
+    /* Failed attempt to make the test print soft references it has trouble clearing. The test ultimately passes, but cannot find the soft references via any root path.
+    private static void assertGC(WeakReference<?> reference) throws Exception {
+        assertTrue(true); reference.get(); // preload any needed classes!
+        Set<Object[]> objects = new HashSet<Object[]>();
+        int size = 1024;
+        while (reference.get() != null) {
+            LiveEngine e = new LiveEngine();
+            // The default filter, ScannerUtils.skipNonStrongReferencesFilter(), omits SoftReference.referent that we care about.
+            // The constructor accepting a filter ANDs it with the default filter, making it useless for this purpose.
+            Field f = LiveEngine.class.getDeclaredField("filter");
+            f.setAccessible(true);
+            f.set(e, new Filter() {
+                final Field referent = Reference.class.getDeclaredField("referent");
+                @Override public boolean accept(Object obj, Object referredFrom, Field reference) {
+                    return !(referent.equals(reference) && referredFrom instanceof WeakReference);
+                }
+            });
+            System.err.println(e.trace(Collections.singleton(reference.get()), null));
+            System.err.println("allocating " + size);
+            try {
+                objects.add(new Object[size]);
+            } catch (OutOfMemoryError ignore) {
+                break;
+            }
+            size *= 1.1;
+            System.gc();
+        }
+        objects = null;
+        System.gc();
+        Object obj = reference.get();
+        if (obj != null) {
+            fail(LiveReferences.fromRoots(Collections.singleton(obj)).toString());
+        }
+    }
+    */
+
+}

cps/pom.xml

@@ -75,7 +75,7 @@
         <dependency>
             <groupId>com.cloudbees</groupId>
             <artifactId>groovy-cps</artifactId>
-            <version>1.1</version>
+            <version>1.2</version>
         </dependency>
     </dependencies>
     <!-- TODO currently job depends on tests from here; this dependency should probably be broken -->

cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java

@@ -33,7 +33,6 @@
 import com.cloudbees.groovy.cps.sandbox.DefaultInvoker;
 import com.cloudbees.groovy.cps.sandbox.SandboxInvoker;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Maps;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
@@ -89,19 +88,20 @@
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
-import java.util.concurrent.Phaser;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.thoughtworks.xstream.io.ExtendedHierarchicalStreamWriterHelper.*;
 import hudson.model.User;
 import hudson.security.ACL;
+import java.beans.Introspector;
 import javax.annotation.CheckForNull;
 import javax.annotation.concurrent.GuardedBy;
 
 import org.acegisecurity.Authentication;
 import org.acegisecurity.userdetails.UsernameNotFoundException;
+import org.jboss.marshalling.reflect.SerializableClassRegistry;
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
 
 /**
@@ -247,8 +247,11 @@ public class CpsFlowExecution extends FlowExecution {
     /**
      * Groovy compiler with CPS+sandbox transformation correctly setup.
      * By the time the script starts running, this field is set to non-null.
+     * It is reset to null after completion.
      */
     private transient CpsGroovyShell shell;
+    /** Class of the {@link CpsScript}; its loader is a {@link groovy.lang.GroovyClassLoader.InnerLoader}, not the same as {@code shell.getClassLoader()}. */
+    private transient Class<?> scriptClass;
 
     /** Actions to add to the {@link FlowStartNode}. */
     transient final List<Action> flowStartNodeActions = new ArrayList<Action>();
@@ -315,6 +318,7 @@ public File getStorageDir() throws IOException {
     @Override
     public void start() throws IOException {
         final CpsScript s = parseScript();
+        scriptClass = s.getClass();
         s.initialize();
 
         final FlowHead h = new FlowHead(this);
@@ -398,9 +402,9 @@ public void loadProgramAsync(File programDataFile) {
         programPromise = result;
 
         try {
-            final ClassLoader scriptClassLoader = parseScript().getClass().getClassLoader();
+            scriptClass = parseScript().getClass();
 
-            RiverReader r = new RiverReader(programDataFile, scriptClassLoader, owner);
+            RiverReader r = new RiverReader(programDataFile, scriptClass.getClassLoader(), owner);
             Futures.addCallback(
                     r.restorePickles(),
 
@@ -683,6 +687,12 @@ public boolean isComplete() {
         first.setNewHead(head);
         heads.clear();
         heads.put(first.getId(),first);
+
+        // clean up heap
+        shell = null;
+        SerializableClassRegistry.getInstance().release(scriptClass.getClassLoader());
+        Introspector.flushFromCaches(scriptClass); // does not handle other derived script classes, but this is only SoftReference anyway
+        scriptClass = null;
     }
 
     synchronized FlowHead getFirstHead() {