Skip to content

Commit

Permalink
Merge pull request #759 from jglick/restartWhileTemporarilyPaused-JEN…
Browse files Browse the repository at this point in the history 
…KINS-59743

[JENKINS-59743] Restarting before `afterStepExecutionsResumed` could leave a build paused
  • Loading branch information
@jglick
jglick authored Aug 2, 2023
2 parents d5c88ee + 929dfcd commit e4b5b85
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 22 deletions.
4 changes: 2 additions & 2 deletions plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* When this method is called, the 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();
Expand Down
101 changes: 85 additions & 16 deletions plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,32 +24,44 @@

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;
import java.util.ArrayList;
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;
Expand All @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -352,6 +357,70 @@ private static List<String> stepNames(ListenableFuture<List<StepExecution>> 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<? extends Class<?>> getRequiredContext() {
return Set.of(TaskListener.class);
}
}
}

@Test public void timing() throws Throwable {
sessions.then(r -> {
logger.record(CpsFlowExecution.TIMING_LOGGER, Level.FINE).capture(100);
Expand Down

0 comments on commit e4b5b85

Please sign in to comment.