Merge pull request #53 from jglick/thread-pool

Use a dedicated thread pool rather than Timer.get for DurableTaskStep.Execution.check
jenkinsci · Oct 13, 2017 · bd45d03 · bd45d03
2 parents 36fccc8 + 6ef007e
commit bd45d03
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 16 deletions.
diff --git a/Jenkinsfile b/Jenkinsfile
@@ -1 +1 @@
-buildPlugin(jenkinsVersions: [null, '2.60.1'])
+buildPlugin(jenkinsVersions: [null, '2.73.1'])
diff --git a/pom.xml b/pom.xml
@@ -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>
@@ -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>
@@ -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>
@@ -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>
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java
@@ -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;
@@ -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;
 
@@ -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;
@@ -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);
                 }
             }
         }
@@ -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;

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/BuildQueueTasksTest.java b/src/test/java/org/jenkinsci/plugins/workflow/BuildQueueTasksTest.java
@@ -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;
@@ -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);
@@ -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
             }
@@ -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);
@@ -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");
 
@@ -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());