[FIXED JENKINS-31614] Avoid acquiring the Queue lock while holding ot…

…her locks.
jenkinsci · Jan 5, 2016 · c89a054 · c89a054
1 parent be247cb
commit c89a054
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 20 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -5,6 +5,7 @@ Only noting significant user changes, not internal code cleanups and minor bug f
 ## 1.13 (upcoming)
 
 * [JENKINS-26126](https://issues.jenkins-ci.org/browse/JENKINS-26126) (partial): introspect Workflow steps to generate static reference documentation (link from _Snippet Generator_). Planned to be used for IDE auto-completion as well.
+* [JENKINS-31614](https://issues.jenkins-ci.org/browse/JENKINS-31614): avoiding various deadlocks involving `Queue`.
 * [JENKINS-31897](https://issues.jenkins-ci.org/browse/JENKINS-31897): parameters with default values may now be omitted from the `parameters` option to the `build` step.
 * [JENKINS-31909](https://issues.jenkins-ci.org/browse/JENKINS-31909): form validation warning about Groovy syntax errors was broken in 1.11.
 * [JENKINS-31391](https://issues.jenkins-ci.org/browse/JENKINS-31391): pass `EXECUTOR_NUMBER` into `node {}`.

diff --git a/api/src/main/java/org/jenkinsci/plugins/workflow/pickles/Pickle.java b/api/src/main/java/org/jenkinsci/plugins/workflow/pickles/Pickle.java
@@ -39,6 +39,7 @@ public abstract class Pickle implements Serializable {
     /**
      * Start preparing rehydration of this value, and when it's ready or fail, report that to the
      * given call.
+     * An implementation should return quickly and avoid acquiring locks in this method itself (as opposed to the future).
      */
     public abstract ListenableFuture<?> rehydrate();
 

diff --git a/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
@@ -474,7 +474,11 @@ private String key() {
                     listener = new StreamBuildListener(new NullStream());
                 }
                 completed = new AtomicBoolean();
-                Queue.getInstance().schedule(new AfterRestartTask(this), 0);
+                Timer.get().submit(new Runnable() { // JENKINS-31614
+                    @Override public void run() {
+                        Queue.getInstance().schedule(new AfterRestartTask(WorkflowRun.this), 0);
+                    }
+                });
             }
         }
         checkouts(null); // only for diagnostics

diff --git a/support/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/ExecutorPickle.java b/support/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/ExecutorPickle.java
@@ -25,7 +25,6 @@
 package org.jenkinsci.plugins.workflow.support.pickles;
 
 import org.jenkinsci.plugins.workflow.pickles.Pickle;
-import org.jenkinsci.plugins.workflow.support.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import hudson.Extension;
 import hudson.model.Executor;
@@ -62,17 +61,19 @@ private ExecutorPickle(Executor executor) {
     }
 
     @Override public ListenableFuture<Executor> rehydrate() {
-        Queue.Item item = Queue.getInstance().schedule2(task, 0).getItem();
-        if (item == null) {
-            // TODO should also report when !ScheduleResult.created, since that is arguably an error
-            return Futures.immediateFailedFuture(new IllegalStateException("queue refused " + task));
-        }
-
-        final Future<Queue.Executable> future = item.getFuture().getStartCondition();
-
-        return new TryRepeatedly<Executor>(1) {
+        return new TryRepeatedly<Executor>(1, 0) {
+            Future<Queue.Executable> future;
             @Override
             protected Executor tryResolve() throws Exception {
+                if (future == null) {
+                    Queue.Item item = Queue.getInstance().schedule2(task, 0).getItem();
+                    if (item == null) {
+                        // TODO should also report when !ScheduleResult.created, since that is arguably an error
+                        throw new IllegalStateException("queue refused " + task);
+                    }
+                    future = item.getFuture().getStartCondition();
+                }
+
                 if (!future.isDone()) {
                     return null;
                 }

diff --git a/support/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/TryRepeatedly.java b/support/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/TryRepeatedly.java
@@ -38,15 +38,19 @@
  * @author Kohsuke Kawaguchi
  */
 public abstract class TryRepeatedly<V> extends AbstractFuture<V> {
-    private final int seconds;
+    private final int delay;
     private ScheduledFuture<?> next;
 
-    protected TryRepeatedly(int seconds) {
-        this.seconds = seconds;
-        tryLater();
+    protected TryRepeatedly(int delay) {
+        this(delay, delay);
     }
 
-    private void tryLater() {
+    protected TryRepeatedly(int delay, int initialDelay) {
+        this.delay = delay;
+        tryLater(initialDelay);
+    }
+
+    private void tryLater(int currentDelay) {
         // TODO log a warning if trying for too long; probably Pickle.rehydrate should be given a TaskListener to note progress
 
         if (isCancelled())      return;
@@ -57,14 +61,14 @@ public void run() {
                 try {
                     V v = tryResolve();
                     if (v == null)
-                        tryLater();
+                        tryLater(delay);
                     else
                         set(v);
                 } catch (Throwable t) {
                     setException(t);
                 }
             }
-        }, seconds, TimeUnit.SECONDS);
+        }, currentDelay, TimeUnit.SECONDS);
     }
 
     @Override

diff --git a/...ort/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java b/...ort/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java
@@ -348,8 +348,13 @@ private static void finish(@CheckForNull String cookie) {
                     LOGGER.log(FINE, "no running task corresponds to {0}", cookie);
                     return;
                 }
-                assert runningTask.execution != null && runningTask.launcher != null;
-                runningTask.execution.completed(null);
+                final AsynchronousExecution execution = runningTask.execution;
+                assert execution != null && runningTask.launcher != null;
+                Timer.get().submit(new Runnable() { // JENKINS-31614
+                    @Override public void run() {
+                        execution.completed(null);
+                    }
+                });
                 try {
                     runningTask.launcher.kill(Collections.singletonMap(COOKIE_VAR, cookie));
                 } catch (ChannelClosedException x) {