Merge pull request #68 from jglick/starvation-JENKINS-25890

[JENKINS-25890] Use own thread pool for CpsStepContext.getProgramPromise Originally-Committed-As: 729984d402ebae38c2dc44ea3d72eb5f01aa6dfe
jenkinsci · Mar 4, 2015 · 5abe16b · 5abe16b
2 parents 2cd16e2 + bb91f46
commit 5abe16b
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 15 deletions.
diff --git a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
@@ -427,6 +427,7 @@ public void onSuccess(Unmarshaller u) {
                         }
 
                         public void onFailure(Throwable t) {
+                            // Note: not calling result.setException(t) since loadProgramFailed in fact sets a result
                             try {
                                 loadProgramFailed(t, result);
                             } finally {

diff --git a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java b/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java
@@ -31,6 +31,8 @@
 import groovy.lang.Closure;
 import hudson.model.Descriptor;
 import hudson.model.Result;
+import hudson.util.DaemonThreadFactory;
+import hudson.util.NamingThreadFactory;
 import jenkins.model.Jenkins;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepEndNode;
 import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
@@ -57,9 +59,11 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import jenkins.util.Timer;
+import jenkins.util.ContextResettingExecutorService;
 import org.codehaus.groovy.runtime.InvokerInvocationException;
 
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
@@ -160,6 +164,12 @@ public class CpsStepContext extends DefaultStepContext { // TODO add XStream cla
      */
     private transient volatile StepDescriptor stepDescriptor;
 
+    /**
+     * Cached value of {@link #getProgramPromise}.
+     * Never null once set (might be overwritten).
+     */
+    private transient volatile ListenableFuture<CpsThreadGroup> programPromise;
+
     @CpsVmThreadOnly
     CpsStepContext(StepDescriptor step, CpsThread thread, FlowExecutionOwner executionRef, FlowNode node, Closure body) {
         this.threadId = thread.id;
@@ -224,22 +234,26 @@ public String getDisplayName() {
             throw new IOException(e);
         }
     }
-
+
+    private static final ExecutorService getProgramPromiseExecutorService = new ContextResettingExecutorService(Executors.newCachedThreadPool(new NamingThreadFactory(new DaemonThreadFactory(), "CpsStepContext.getProgramPromise")));
     private @Nonnull ListenableFuture<CpsThreadGroup> getProgramPromise() {
-        final SettableFuture<CpsThreadGroup> f = SettableFuture.create();
-        // TODO is there some more convenient way of writing this using Futures.transform?
-        Timer.get().submit(new Runnable() {
-            @Override public void run() {
-                try {
-                    ListenableFuture<CpsThreadGroup> pp = getFlowExecution().programPromise;
-                    assert pp != null;
-                    f.set(pp.get());
-                } catch (Exception x) { // from getFlowExecution() or get()
-                    f.setException(x);
+        if (programPromise == null) {
+            final SettableFuture<CpsThreadGroup> f = SettableFuture.create();
+            // TODO is there some more convenient way of writing this using Futures.transform? FlowExecutionOwner.get should really return ListenableFuture<FlowExecution>
+            getProgramPromiseExecutorService.submit(new Runnable() {
+                @Override public void run() {
+                    try {
+                        ListenableFuture<CpsThreadGroup> pp = getFlowExecution().programPromise;
+                        assert pp != null;
+                        f.set(pp.get());
+                    } catch (Exception x) { // from getFlowExecution() or get()
+                        f.setException(x);
+                    }
                 }
-            }
-        });
-        return f;
+            });
+            programPromise = f;
+        }
+        return programPromise;
     }
 
     @Override public boolean isReady() {