Merge pull request #289 from jglick/deadlocks-JENKINS-31614

[JENKINS-31614] Avoid acquiring the Queue lock while holding other locks

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 {}`.

api/src/main/java/org/jenkinsci/plugins/workflow/pickles/Pickle.java

@@ -39,6 +39,7 @@
     /**
      * 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();
 

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

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;
                 }

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

support/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) {