diff --git a/CHANGES.md b/CHANGES.md index 870ccd518..c301e0978 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,6 +7,7 @@ Only noting significant user-visible or major API changes, not internal code cle ### User changes * Ability to configure project display name. * Fixing `java.io.NotSerializableException: org.jenkinsci.plugins.workflow.support.steps.StageStepExecution$CanceledCause` thrown from certain scripts using `stage`. +* PR 52: fixed some memory leaks causing the permanent generation and heap to grow unbounded after many flow builds. ## 1.2 (Jan 24 2015) diff --git a/aggregator/src/test/java/org/jenkinsci/plugins/workflow/CpsFlowExecutionTest.java b/aggregator/src/test/java/org/jenkinsci/plugins/workflow/CpsFlowExecutionTest.java new file mode 100644 index 000000000..529ed874c --- /dev/null +++ b/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 LOADER; + public static void register(Object o) { + LOADER = new WeakReference(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 objects = new HashSet(); + 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()); + } + } + */ + +} diff --git a/cps/pom.xml b/cps/pom.xml index 57956d8ce..bf78576f4 100644 --- a/cps/pom.xml +++ b/cps/pom.xml @@ -75,7 +75,7 @@ com.cloudbees groovy-cps - 1.1 + 1.2 diff --git a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index b60bda5e4..df989873b 100644 --- a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/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,7 +88,6 @@ 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; @@ -97,11 +95,13 @@ 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 flowStartNodeActions = new ArrayList(); @@ -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() {