Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a dedicated thread pool rather than Timer.get for DurableTaskStep.Execution.check #53

Merged
merged 5 commits into from
Oct 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Jenkinsfile
Original file line number Diff line number Diff line change
@@ -1 +1 @@
buildPlugin(jenkinsVersions: [null, '2.60.1'])
buildPlugin(jenkinsVersions: [null, '2.73.1'])
16 changes: 11 additions & 5 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>2.30</version>
<version>2.36</version>
<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -62,7 +62,8 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>2.7.3</jenkins.version>
<jenkins.version>2.60.3</jenkins.version>
<java.level>8</java.level>
<workflow-step-api-plugin.version>2.11</workflow-step-api-plugin.version>
</properties>
<dependencies>
Expand All @@ -79,12 +80,12 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.20</version>
<version>2.22</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
<version>2.13</version>
<version>2.16-20170927.202130-1</version> <!-- TODO https://github.com/jenkinsci/workflow-support-plugin/pull/46 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -133,7 +134,12 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.25</version>
<version>1.27</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.7</version>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@
import hudson.FilePath;
import hudson.Launcher;
import hudson.model.TaskListener;
import hudson.util.DaemonThreadFactory;
import hudson.util.FormValidation;
import hudson.util.LogTaskListener;
import hudson.util.NamingThreadFactory;
import java.io.IOException;
import java.io.PrintStream;
import java.nio.charset.Charset;
import java.util.Set;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -53,6 +56,7 @@
import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
import org.jenkinsci.plugins.workflow.steps.StepExecution;
import org.jenkinsci.plugins.workflow.support.concurrent.Timeout;
import org.jenkinsci.plugins.workflow.support.concurrent.WithThreadName;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;

Expand Down Expand Up @@ -136,6 +140,12 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
private static final long MAX_RECURRENCE_PERIOD = 15000; // 15s
private static final float RECURRENCE_PERIOD_BACKOFF = 1.2f;

private static final ScheduledThreadPoolExecutor THREAD_POOL = new ScheduledThreadPoolExecutor(25, new NamingThreadFactory(new DaemonThreadFactory(), DurableTaskStep.class.getName()));
static {
THREAD_POOL.setKeepAliveTime(1, TimeUnit.MINUTES);
THREAD_POOL.allowCoreThreadTimeOut(true);
}

private transient final DurableTaskStep step;
private transient FilePath ws;
private transient long recurrencePeriod;
Expand Down Expand Up @@ -275,11 +285,13 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
/** Checks for progress or completion of the external task. */
@Override public void run() {
task = null;
try {
try (WithThreadName naming = new WithThreadName(": checking " + remote + " on " + node)) {
check();
} catch (Exception x) { // TODO use ErrorLoggingScheduledThreadPoolExecutor from core if it becomes public
LOGGER.log(Level.WARNING, null, x);
} finally {
if (recurrencePeriod > 0) {
task = Timer.get().schedule(this, recurrencePeriod, TimeUnit.MILLISECONDS);
task = THREAD_POOL.schedule(this, recurrencePeriod, TimeUnit.MILLISECONDS);
}
}
}
Expand Down Expand Up @@ -341,7 +353,7 @@ private void check() {

private void setupTimer() {
recurrencePeriod = MIN_RECURRENCE_PERIOD;
task = Timer.get().schedule(this, recurrencePeriod, TimeUnit.MILLISECONDS);
task = THREAD_POOL.schedule(this, recurrencePeriod, TimeUnit.MILLISECONDS);
}

private static final long serialVersionUID = 1L;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import java.util.concurrent.ExecutionException;

import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
Expand Down Expand Up @@ -61,7 +60,7 @@ public class BuildQueueTasksTest {
@Override public void evaluate() throws Throwable {
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
// use non-existent node label to keep the build queued
p.setDefinition(new CpsFlowDefinition("node('nonexistent') { echo 'test' }"));
p.setDefinition(new CpsFlowDefinition("node('nonexistent') { echo 'test' }", true));

WorkflowRun b = scheduleAndWaitQueued(p);
assertQueueAPIStatusOKAndAbort(b);
Expand All @@ -76,7 +75,7 @@ public class BuildQueueTasksTest {
@Override public void evaluate() throws Throwable {
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
// use non-existent node label to keep the build queued
p.setDefinition(new CpsFlowDefinition("node('nonexistent') { echo 'test' }"));
p.setDefinition(new CpsFlowDefinition("node('nonexistent') { echo 'test' }", true));
scheduleAndWaitQueued(p);
// Ok, the item is in he queue now, restart
}
Expand All @@ -101,7 +100,7 @@ public class BuildQueueTasksTest {
"node {\n" +
" echo 'test'\n " +
" semaphore 'watch'\n " +
"}"));
"}", true));

WorkflowRun b = p.scheduleBuild2(0).getStartCondition().get();
SemaphoreStep.waitForStart("watch/1", b);
Expand Down Expand Up @@ -132,7 +131,7 @@ private WorkflowRun scheduleAndWaitQueued(WorkflowJob p) throws InterruptedExcep
}

private void assertQueueAPIStatusOKAndAbort(WorkflowRun b)
throws IOException, SAXException, InterruptedException, ExecutionException {
throws Exception {
JenkinsRule.WebClient wc = story.j.createWebClient();
Page queue = wc.goTo("queue/api/json", "application/json");

Expand All @@ -142,8 +141,8 @@ private void assertQueueAPIStatusOKAndAbort(WorkflowRun b)
// Not going into de the content in this test
assertEquals(1, items.size());

CpsFlowExecution e = (CpsFlowExecution) b.getExecutionPromise().get();
e.interrupt(Result.ABORTED);
b.getExecutor().interrupt();
story.j.assertBuildStatus(Result.ABORTED, story.j.waitForCompletion(b));

queue = wc.goTo("queue/api/json", "application/json");
o = JSONObject.fromObject(queue.getWebResponse().getContentAsString());
Expand Down