[JENKINS-75067] Unexport step bodies when completed in synchronous mo…

…de (#966) * [JENKINS-75067] Unexport step bodies when completed in synchronous mode Previously, the unexport of a step's body would only happen when scheduling the next execution (async mode). If the step was not run async (due to having an outcome set prior to the step starting), the body was not removed until the CpsThreadGroup was finishing. At this point the body's presence was treated as a resource leak and logged a warning. Doing an explicit unexport for the sync case at the proper time eliminates the warnings from the fall-back cleanup. * [JENKINS-58084] Make comment more informative * [JENKINS-58084] Add test cases There are two additional confirmed scenarios where closures were leaked, both of which are covered by this fix. As these new tests have nothing to do with `StepListener` (and to keep the tests together), they have all moved and `StepListenerTest` has been effectively reverted to its previous state. * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <[email protected]> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <[email protected]> --------- Co-authored-by: Devin Nusbaum <[email protected]> Co-authored-by: Jesse Glick <[email protected]>
jenkinsci · Jan 3, 2025 · f9c5614 · f9c5614
1 parent 80cad0f
commit f9c5614
Show file tree

Hide file tree

Showing 2 changed files with 191 additions and 0 deletions.
diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java
@@ -359,6 +359,13 @@ private static boolean refersTo(Throwable t1, Throwable t2) {
      */
     private void scheduleNextRun() {
         if (syncMode) {
+            // probably rare for a legit sync step to have a body (unless short-circuiting execution of the body, as
+            // running a body in sync mode is not allowed), but it's possible for a (typically) async step to be
+            // *treated* as sync due to having an outcome set prematurely (e.g. from a StepListener)
+            if (threadGroup != null && body != null) {
+                threadGroup.unexport(body);
+                body = null;
+            }
             // if we get the result set before the start method returned, then DSL.invokeMethod() will
             // plan the next action.
             return;

diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java
@@ -0,0 +1,184 @@
+package org.jenkinsci.plugins.workflow.cps;
+
+import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.AbortException;
+import hudson.ExtensionList;
+import hudson.model.Result;
+import org.jenkinsci.plugins.workflow.flow.GraphListener;
+import org.jenkinsci.plugins.workflow.flow.StepListener;
+import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
+import org.jenkinsci.plugins.workflow.graph.FlowNode;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+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.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.BuildWatcher;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.DataBoundConstructor;
+
+import java.util.Set;
+import java.util.logging.Level;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
+
+public class CpsStepContextTest {
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+
+    @ClassRule
+    public static BuildWatcher buildWatcher = new BuildWatcher();
+
+    @Rule
+    public LoggerRule logger = new LoggerRule();
+
+    @Issue("JENKINS-75067")
+    @Test
+    public void failingStepListenerNotLeakClosures() throws Exception {
+        // Even before the fix there's only one warning logged.  Asserting zero records is probably over-stepping,
+        // but asserting just one record with our target message risks a false negative (some other unrelated message
+        // being first, and our being later).
+        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
+        WorkflowJob job = r.createProject(WorkflowJob.class, "p");
+        job.setDefinition(new CpsFlowDefinition("node {}", true));
+
+        WorkflowRun build = r.buildAndAssertStatus(Result.FAILURE, job);
+        r.assertLogContains("oops", build);
+        assertThat(ClosureCounter.get().closureCount, equalTo(0));
+        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
+    }
+
+    @TestExtension("failingStepListenerNotLeakClosures")
+    public static class FailingStepListener implements StepListener {
+
+        @Override
+        public void notifyOfNewStep(@NonNull Step s, @NonNull StepContext context) {
+            context.onFailure(new AbortException("oops"));
+        }
+    }
+
+    @Issue("JENKINS-75067")
+    @Test
+    public void executionStartExceptionNotLeakClosures() throws Exception {
+        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
+        WorkflowJob job = r.createProject(WorkflowJob.class, "p");
+        job.setDefinition(new CpsFlowDefinition("badBlock {}", true));
+
+        WorkflowRun build = r.buildAndAssertStatus(Result.FAILURE, job);
+        r.assertLogContains("oops", build);
+        assertThat(ClosureCounter.get().closureCount, equalTo(0));
+        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
+    }
+
+    @Issue("JENKINS-75067")
+    @Test
+    public void executionWithBodyRunningSyncNotLeakClosures() throws Exception {
+        logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
+        WorkflowJob job = r.createProject(WorkflowJob.class, "p");
+        job.setDefinition(new CpsFlowDefinition("def r = passthrough {}; echo r", true));
+
+        WorkflowRun build = r.buildAndAssertSuccess(job);
+        r.assertLogContains("hooray", build);
+        assertThat(ClosureCounter.get().closureCount, equalTo(0));
+        assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
+    }
+
+    public static class BadBlockStep extends Step {
+
+        @DataBoundConstructor
+        public BadBlockStep() {}
+
+        @Override
+        public StepExecution start(StepContext context) throws Exception {
+        return StepExecutions.synchronous(context, ctx -> {
+            throw new AbortException("oops");
+        });
+        }
+
+        @TestExtension("executionStartExceptionNotLeakClosures")
+        public static class DescriptorImpl extends StepDescriptor {
+
+            @Override
+            public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of();
+            }
+
+            @Override
+            public String getFunctionName() {
+                return "badBlock";
+            }
+
+            @Override
+            public boolean takesImplicitBlockArgument() {
+                return true;
+            }
+        }
+    }
+
+    public static class PassthroughStep extends Step {
+
+        @DataBoundConstructor
+        public PassthroughStep() {}
+
+        @Override
+        public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.synchronous(context, ctx -> {
+                return "hooray";
+            });
+        }
+
+        @TestExtension("executionWithBodyRunningSyncNotLeakClosures")
+        public static class DescriptorImpl extends StepDescriptor {
+
+            @Override
+            public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of();
+            }
+
+            @Override
+            public String getFunctionName() {
+                return "passthrough";
+            }
+
+            @Override
+            public boolean takesImplicitBlockArgument() {
+                return true;
+            }
+        }
+    }
+
+    @TestExtension
+    public static class ClosureCounter implements GraphListener.Synchronous {
+        int closureCount = -1;
+
+        @Override
+        public void onNewHead(FlowNode node) {
+            // this only works using a Synchronous listener, otherwise the fall-back closure cleaning
+            // will have already executed prior to receiving this event
+            if (node instanceof FlowEndNode) {
+                try {
+                    closureCount = ((CpsFlowExecution) node.getExecution()).programPromise.get().closures.size();
+                }
+                catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        }
+
+        static ClosureCounter get() {
+            return ExtensionList.lookupSingleton(ClosureCounter.class);
+        }
+    }
+}