Skip to content

Commit f9c5614

Browse files
rrjjvvdwnusbaumjglick
authored
[JENKINS-75067] Unexport step bodies when completed in synchronous mode (#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 <dwnusbaum@users.noreply.github.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <jglick@cloudbees.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <jglick@cloudbees.com> * Update plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java Co-authored-by: Jesse Glick <jglick@cloudbees.com> --------- Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com> Co-authored-by: Jesse Glick <jglick@cloudbees.com>
1 parent 80cad0f commit f9c5614

File tree

2 files changed

+191
-0
lines changed

2 files changed

+191
-0
lines changed

‎plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java

+7
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,13 @@ private static boolean refersTo(Throwable t1, Throwable t2) {
359359
*/
360360
private void scheduleNextRun() {
361361
if (syncMode) {
362+
// probably rare for a legit sync step to have a body (unless short-circuiting execution of the body, as
363+
// running a body in sync mode is not allowed), but it's possible for a (typically) async step to be
364+
// *treated* as sync due to having an outcome set prematurely (e.g. from a StepListener)
365+
if (threadGroup != null && body != null) {
366+
threadGroup.unexport(body);
367+
body = null;
368+
}
362369
// if we get the result set before the start method returned, then DSL.invokeMethod() will
363370
// plan the next action.
364371
return;

‎plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsStepContextTest.java

+184
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
package org.jenkinsci.plugins.workflow.cps;
2+
3+
import edu.umd.cs.findbugs.annotations.NonNull;
4+
import hudson.AbortException;
5+
import hudson.ExtensionList;
6+
import hudson.model.Result;
7+
import org.jenkinsci.plugins.workflow.flow.GraphListener;
8+
import org.jenkinsci.plugins.workflow.flow.StepListener;
9+
import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
10+
import org.jenkinsci.plugins.workflow.graph.FlowNode;
11+
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
12+
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
13+
import org.jenkinsci.plugins.workflow.steps.Step;
14+
import org.jenkinsci.plugins.workflow.steps.StepContext;
15+
import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
16+
import org.jenkinsci.plugins.workflow.steps.StepExecution;
17+
import org.jenkinsci.plugins.workflow.steps.StepExecutions;
18+
import org.junit.ClassRule;
19+
import org.junit.Rule;
20+
import org.junit.Test;
21+
import org.jvnet.hudson.test.BuildWatcher;
22+
import org.jvnet.hudson.test.Issue;
23+
import org.jvnet.hudson.test.JenkinsRule;
24+
import org.jvnet.hudson.test.LoggerRule;
25+
import org.jvnet.hudson.test.TestExtension;
26+
import org.kohsuke.stapler.DataBoundConstructor;
27+
28+
import java.util.Set;
29+
import java.util.logging.Level;
30+
31+
import static org.hamcrest.MatcherAssert.assertThat;
32+
import static org.hamcrest.Matchers.containsString;
33+
import static org.hamcrest.Matchers.equalTo;
34+
import static org.hamcrest.Matchers.hasItem;
35+
import static org.hamcrest.Matchers.not;
36+
37+
public class CpsStepContextTest {
38+
@Rule
39+
public JenkinsRule r = new JenkinsRule();
40+
41+
@ClassRule
42+
public static BuildWatcher buildWatcher = new BuildWatcher();
43+
44+
@Rule
45+
public LoggerRule logger = new LoggerRule();
46+
47+
@Issue("JENKINS-75067")
48+
@Test
49+
public void failingStepListenerNotLeakClosures() throws Exception {
50+
// Even before the fix there's only one warning logged. Asserting zero records is probably over-stepping,
51+
// but asserting just one record with our target message risks a false negative (some other unrelated message
52+
// being first, and our being later).
53+
logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
54+
WorkflowJob job = r.createProject(WorkflowJob.class, "p");
55+
job.setDefinition(new CpsFlowDefinition("node {}", true));
56+
57+
WorkflowRun build = r.buildAndAssertStatus(Result.FAILURE, job);
58+
r.assertLogContains("oops", build);
59+
assertThat(ClosureCounter.get().closureCount, equalTo(0));
60+
assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
61+
}
62+
63+
@TestExtension("failingStepListenerNotLeakClosures")
64+
public static class FailingStepListener implements StepListener {
65+
66+
@Override
67+
public void notifyOfNewStep(@NonNull Step s, @NonNull StepContext context) {
68+
context.onFailure(new AbortException("oops"));
69+
}
70+
}
71+
72+
@Issue("JENKINS-75067")
73+
@Test
74+
public void executionStartExceptionNotLeakClosures() throws Exception {
75+
logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
76+
WorkflowJob job = r.createProject(WorkflowJob.class, "p");
77+
job.setDefinition(new CpsFlowDefinition("badBlock {}", true));
78+
79+
WorkflowRun build = r.buildAndAssertStatus(Result.FAILURE, job);
80+
r.assertLogContains("oops", build);
81+
assertThat(ClosureCounter.get().closureCount, equalTo(0));
82+
assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
83+
}
84+
85+
@Issue("JENKINS-75067")
86+
@Test
87+
public void executionWithBodyRunningSyncNotLeakClosures() throws Exception {
88+
logger.record(CpsThreadGroup.class, Level.WARNING).capture(10);
89+
WorkflowJob job = r.createProject(WorkflowJob.class, "p");
90+
job.setDefinition(new CpsFlowDefinition("def r = passthrough {}; echo r", true));
91+
92+
WorkflowRun build = r.buildAndAssertSuccess(job);
93+
r.assertLogContains("hooray", build);
94+
assertThat(ClosureCounter.get().closureCount, equalTo(0));
95+
assertThat(logger.getMessages(), not(hasItem(containsString("Stale closure"))));
96+
}
97+
98+
public static class BadBlockStep extends Step {
99+
100+
@DataBoundConstructor
101+
public BadBlockStep() {}
102+
103+
@Override
104+
public StepExecution start(StepContext context) throws Exception {
105+
return StepExecutions.synchronous(context, ctx -> {
106+
throw new AbortException("oops");
107+
});
108+
}
109+
110+
@TestExtension("executionStartExceptionNotLeakClosures")
111+
public static class DescriptorImpl extends StepDescriptor {
112+
113+
@Override
114+
public Set<? extends Class<?>> getRequiredContext() {
115+
return Set.of();
116+
}
117+
118+
@Override
119+
public String getFunctionName() {
120+
return "badBlock";
121+
}
122+
123+
@Override
124+
public boolean takesImplicitBlockArgument() {
125+
return true;
126+
}
127+
}
128+
}
129+
130+
public static class PassthroughStep extends Step {
131+
132+
@DataBoundConstructor
133+
public PassthroughStep() {}
134+
135+
@Override
136+
public StepExecution start(StepContext context) throws Exception {
137+
return StepExecutions.synchronous(context, ctx -> {
138+
return "hooray";
139+
});
140+
}
141+
142+
@TestExtension("executionWithBodyRunningSyncNotLeakClosures")
143+
public static class DescriptorImpl extends StepDescriptor {
144+
145+
@Override
146+
public Set<? extends Class<?>> getRequiredContext() {
147+
return Set.of();
148+
}
149+
150+
@Override
151+
public String getFunctionName() {
152+
return "passthrough";
153+
}
154+
155+
@Override
156+
public boolean takesImplicitBlockArgument() {
157+
return true;
158+
}
159+
}
160+
}
161+
162+
@TestExtension
163+
public static class ClosureCounter implements GraphListener.Synchronous {
164+
int closureCount = -1;
165+
166+
@Override
167+
public void onNewHead(FlowNode node) {
168+
// this only works using a Synchronous listener, otherwise the fall-back closure cleaning
169+
// will have already executed prior to receiving this event
170+
if (node instanceof FlowEndNode) {
171+
try {
172+
closureCount = ((CpsFlowExecution) node.getExecution()).programPromise.get().closures.size();
173+
}
174+
catch (Exception e) {
175+
throw new RuntimeException(e);
176+
}
177+
}
178+
}
179+
180+
static ClosureCounter get() {
181+
return ExtensionList.lookupSingleton(ClosureCounter.class);
182+
}
183+
}
184+
}

0 commit comments

Comments
 (0)