From 9ff0e16bc4fe7f55043ed305ca0d2df3f51b0485 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 16 Sep 2024 16:07:16 -0400 Subject: [PATCH] Add test for memory leak through StepExecutionIterator when build is stuck loading programPromise --- .gitignore | 3 + .../workflow/flow/FlowExecutionListTest.java | 74 ++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 948c7a3e..4d461dd7 100644 --- a/.gitignore +++ b/.gitignore @@ -26,3 +26,6 @@ tags # mvn versions:set pom.xml.versionsBackup + +# MemoryAssert tests +*.hprof diff --git a/src/test/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionListTest.java b/src/test/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionListTest.java index 9952dadf..de9d1fb4 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionListTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionListTest.java @@ -30,7 +30,9 @@ import static org.hamcrest.Matchers.hasItem; import static org.junit.Assert.assertNotNull; +import com.google.common.util.concurrent.ListenableFuture; import hudson.AbortException; +import hudson.ExtensionList; import hudson.model.ParametersAction; import hudson.model.ParametersDefinitionProperty; import hudson.model.Result; @@ -53,11 +55,14 @@ import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.pickles.Pickle; import org.jenkinsci.plugins.workflow.steps.Step; import org.jenkinsci.plugins.workflow.steps.StepContext; import org.jenkinsci.plugins.workflow.steps.StepDescriptor; import org.jenkinsci.plugins.workflow.steps.StepExecution; import org.jenkinsci.plugins.workflow.steps.StepExecutions; +import org.jenkinsci.plugins.workflow.support.pickles.SingleTypedPickleFactory; +import org.jenkinsci.plugins.workflow.support.pickles.TryRepeatedly; import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; import org.junit.ClassRule; import org.junit.Test; @@ -197,7 +202,7 @@ public class FlowExecutionListTest { }); } - @Test public void stepExecutionIteratorDoesNotLeakBuildsWhenOneIsStuck() throws Throwable { + @Test public void stepExecutionIteratorDoesNotLeakBuildsWhenCpsVmIsStuck() throws Throwable { sessions.then(r -> { var notStuck = r.createProject(WorkflowJob.class, "not-stuck"); notStuck.setDefinition(new CpsFlowDefinition("semaphore 'wait'", true)); @@ -225,6 +230,42 @@ public class FlowExecutionListTest { }); } + @Test public void stepExecutionIteratorDoesNotLeakBuildsWhenProgramPromiseIsStuck() throws Throwable { + sessions.then(r -> { + var stuck = r.createProject(WorkflowJob.class, "stuck"); + stuck.setDefinition(new CpsFlowDefinition( + "def x = new org.jenkinsci.plugins.workflow.flow.FlowExecutionListTest.StuckPickle.Marker()\n" + + "semaphore 'stuckWait'\n" + + "echo x.getClass().getName()", false)); + var stuckBuild = stuck.scheduleBuild2(0).waitForStart(); + SemaphoreStep.waitForStart("stuckWait/1", stuckBuild); + }); + sessions.then(r -> { + var notStuck = r.createProject(WorkflowJob.class, "not-stuck"); + notStuck.setDefinition(new CpsFlowDefinition("semaphore 'wait'", true)); + var notStuckBuild = notStuck.scheduleBuild2(0).waitForStart(); + SemaphoreStep.waitForStart("wait/1", notStuckBuild); + WeakReference notStuckBuildRef = new WeakReference<>(notStuckBuild); + var stuck = r.jenkins.getItemByFullName("stuck", WorkflowJob.class); + var stuckBuild = stuck.getBuildByNumber(1); + // Make FlowExecutionList$StepExecutionIteratorImpl.applyAll submit a task to the CpsVmExecutorService + // for stuck #1 that will never complete, so the resulting future will never complete. + StepExecution.applyAll(e -> null); + // Let notStuckBuild complete and clean up all references. + SemaphoreStep.success("wait/1", null); + r.waitForCompletion(notStuckBuild); + notStuckBuild = null; // Clear out the local variable in this thread. + Jenkins.get().getQueue().clearLeftItems(); // Otherwise we'd have to wait 5 minutes for the cache to be cleared. + // Make sure that the reference can be GC'd. + MemoryAssert.assertGC(notStuckBuildRef, true); + // Allow stuck #1 to complete so the test can be cleaned up promptly. + r.waitForMessage("Still trying to load StuckPickle for", stuckBuild); + ExtensionList.lookupSingleton(StuckPickle.Factory.class).resolved = new StuckPickle.Marker(); + SemaphoreStep.success("stuckWait/1", null); + r.waitForCompletion(stuckBuild); + }); + } + public static class NonResumableStep extends Step implements Serializable { public static final long serialVersionUID = 1L; @DataBoundConstructor @@ -306,7 +347,7 @@ private enum State { UNBLOCKED, } - @TestExtension("stepExecutionIteratorDoesNotLeakBuildsWhenOneIsStuck") public static class DescriptorImpl extends StepDescriptor { + @TestExtension("stepExecutionIteratorDoesNotLeakBuildsWhenCpsVmIsStuck") public static class DescriptorImpl extends StepDescriptor { @Override public Set> getRequiredContext() { return Collections.singleton(TaskListener.class); @@ -318,4 +359,33 @@ public String getFunctionName() { } } + public static class StuckPickle extends Pickle { + @Override + public ListenableFuture rehydrate(FlowExecutionOwner owner) { + return new TryRepeatedly(1) { + @Override + protected Marker tryResolve() { + return ExtensionList.lookupSingleton(Factory.class).resolved; + } + @Override protected FlowExecutionOwner getOwner() { + return owner; + } + @Override public String toString() { + return "StuckPickle for " + owner; + } + }; + } + + public static class Marker {} + + @TestExtension("stepExecutionIteratorDoesNotLeakBuildsWhenProgramPromiseIsStuck") + public static final class Factory extends SingleTypedPickleFactory { + public Marker resolved; + + @Override protected Pickle pickle(Marker object) { + return new StuckPickle(); + } + } + } + }