From dbee576d391ef31857a3a3ddb19edf8cc101ee6b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 1 Aug 2023 17:52:01 -0400 Subject: [PATCH 1/2] [JENKINS-59743] Restarting before `afterStepExecutionsResumed` could leave a build paused --- .../workflow/cps/CpsFlowExecution.java | 4 +- .../plugins/workflow/cps/CpsThreadGroup.java | 8 +- .../workflow/cps/CpsFlowExecutionTest.java | 101 +++++++++++++++--- 3 files changed, 92 insertions(+), 21 deletions(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 52592b17e..c993d3c5a 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -796,7 +796,7 @@ public void onSuccess(Unmarshaller u) { CpsThreadGroup g = (CpsThreadGroup) u.readObject(); result.set(g); pausedWhenLoaded = g.isPaused(); - g.pause(); + g.pause(false); } catch (Throwable t) { onFailure(t); } finally { @@ -1596,7 +1596,7 @@ public void pause(final boolean v) throws IOException { Futures.addCallback(programPromise, new FutureCallback<>() { @Override public void onSuccess(CpsThreadGroup g) { if (v) { - g.pause(); + g.pause(true); checkAndAbortNonresumableBuild(); // TODO Verify if we can rely on just killing paused builds at shutdown via checkAndAbortNonresumableBuild() checkpoint(false); } else { diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java index 11c1712b3..c4be3eee7 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java @@ -363,14 +363,16 @@ public void run() { /** * Pauses the execution. - * + * @param persist whether this is a user-initiated pause that should be persisted * @return * {@link Future} object that represents the actual suspension of the CPS VM. * When the {@link #pause()} method is called, CPS VM might be still executing. */ - public Future pause() { + public Future pause(boolean persist) { paused.set(true); - executionPaused = true; + if (persist) { + executionPaused = true; + } // CPS VM might have a long queue in its task list, so to properly ensure // that the execution has actually suspended, call scheduleRun() excessively return scheduleRun(); diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java index 8702eff3b..ca20b6ec8 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java @@ -24,16 +24,25 @@ package org.jenkinsci.plugins.workflow.cps; -import org.htmlunit.ElementNotFoundException; -import org.htmlunit.FailingHttpStatusCodeException; -import org.htmlunit.HttpMethod; -import org.htmlunit.WebRequest; +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.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import com.google.common.util.concurrent.ListenableFuture; +import edu.umd.cs.findbugs.annotations.CheckForNull; import groovy.lang.GroovyShell; import hudson.AbortException; +import hudson.ExtensionList; import hudson.model.Item; import hudson.model.Result; import hudson.model.TaskListener; +import java.io.File; import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; @@ -41,15 +50,18 @@ import java.util.Arrays; import java.util.List; import java.util.ListIterator; +import java.util.Set; import java.util.logging.ConsoleHandler; import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.Logger; -import edu.umd.cs.findbugs.annotations.CheckForNull; -import java.io.File; import jenkins.model.Jenkins; import org.apache.commons.io.FileUtils; import org.hamcrest.Matchers; +import org.htmlunit.ElementNotFoundException; +import org.htmlunit.FailingHttpStatusCodeException; +import org.htmlunit.HttpMethod; +import org.htmlunit.WebRequest; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted; import org.jenkinsci.plugins.workflow.cps.GroovySourceFileAllowlist.DefaultAllowlist; @@ -58,21 +70,13 @@ 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.support.pickles.SingleTypedPickleFactory; import org.jenkinsci.plugins.workflow.support.pickles.TryRepeatedly; import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; -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.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -84,6 +88,7 @@ import org.jvnet.hudson.test.LoggerRule; import org.jvnet.hudson.test.MockAuthorizationStrategy; import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.DataBoundConstructor; public class CpsFlowExecutionTest { @@ -352,6 +357,70 @@ private static List stepNames(ListenableFuture> exec }); } + @Issue("JENKINS-59743") + @Test public void restartWhileTemporarilyPaused() throws Throwable { + sessions.then(r -> { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("slowToResume()", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("Started…", b); + }); + sessions.then(r -> { + WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class); + WorkflowRun b = p.getBuildByNumber(1); + r.waitForMessage("Waiting until ready…", b); + }); + sessions.then(r -> { + SlowToResume.DescriptorImpl d = ExtensionList.lookupSingleton(SlowToResume.DescriptorImpl.class); + synchronized (d) { + d.ready = true; + d.notifyAll(); + } + WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class); + WorkflowRun b = p.getBuildByNumber(1); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + } + public static final class SlowToResume extends Step { + @DataBoundConstructor public SlowToResume() {} + @Override public StepExecution start(StepContext context) throws Exception { + return new Execution(context); + } + private static final class Execution extends StepExecution { + Execution(StepContext context) { + super(context); + } + @Override public boolean start() throws Exception { + getContext().get(TaskListener.class).getLogger().println("Started…"); + return false; + } + @Override public void onResume() { + DescriptorImpl d = ExtensionList.lookupSingleton(DescriptorImpl.class); + synchronized (d) { + try { + while (!d.ready) { + getContext().get(TaskListener.class).getLogger().println("Waiting until ready…"); + d.wait(); + } + getContext().get(TaskListener.class).getLogger().println("…ready."); + getContext().onSuccess(null); + } catch (Exception x) { + getContext().onFailure(x); + } + } + } + } + @TestExtension("restartWhileTemporarilyPaused") public static final class DescriptorImpl extends StepDescriptor { + boolean ready; + @Override public String getFunctionName() { + return "slowToResume"; + } + @Override public Set> getRequiredContext() { + return Set.of(TaskListener.class); + } + } + } + @Test public void timing() throws Throwable { sessions.then(r -> { logger.record(CpsFlowExecution.TIMING_LOGGER, Level.FINE).capture(100); From 929dfcd0fd632bcd8896b6bd48ff95dc3c71d949 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 1 Aug 2023 18:21:35 -0400 Subject: [PATCH 2/2] Javadoc --- .../java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java index c4be3eee7..f8e43dd4e 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java @@ -366,7 +366,7 @@ public void run() { * @param persist whether this is a user-initiated pause that should be persisted * @return * {@link Future} object that represents the actual suspension of the CPS VM. - * When the {@link #pause()} method is called, CPS VM might be still executing. + * When this method is called, the CPS VM might be still executing. */ public Future pause(boolean persist) { paused.set(true);