Fix potential deadlock between queue maintenance and asynchronous exe…

…cution (#3354) [JENKINS-46248] - Fix potential deadlock between queue maintenance and asynchronous execution
jenkinsci · Jun 8, 2018 · 01b1f1d · 01b1f1d
1 parent 24ea684
commit 01b1f1d
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 8 deletions.
diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java
@@ -431,11 +431,12 @@ public SubTask call() throws Exception {
             } catch (AsynchronousExecution x) {
                 lock.writeLock().lock();
                 try {
-                    x.setExecutor(this);
+                    x.setExecutorWithoutCompleting(this);
                     this.asynchronousExecution = x;
                 } finally {
                     lock.writeLock().unlock();
                 }
+                x.maybeComplete();
             } catch (Throwable e) {
                 problems = e;
             } finally {

diff --git a/core/src/main/java/jenkins/model/queue/AsynchronousExecution.java b/core/src/main/java/jenkins/model/queue/AsynchronousExecution.java
@@ -39,6 +39,7 @@
 import javax.annotation.concurrent.GuardedBy;
 import jenkins.model.Jenkins;
 import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
@@ -61,7 +62,7 @@ public abstract class AsynchronousExecution extends RuntimeException {
 
     /**
      * Initially null, and usually stays null.
-     * If {@link #completed} is called before {@link #setExecutor}, then either {@link #NULL} for success, or the error.
+     * If {@link #completed} is called before {@link #setExecutorWithoutCompleting}, then either {@link #NULL} for success, or the error.
      */
     @GuardedBy("this")
     private @CheckForNull Throwable result;
@@ -98,21 +99,34 @@ protected AsynchronousExecution() {}
 
     /**
      * Obtains the associated executor.
-     * @return Associated Executor. May be {@code null} if {@link #setExecutor(hudson.model.Executor)} 
+     * @return Associated Executor. May be {@code null} if {@link #setExecutorWithoutCompleting(hudson.model.Executor)} 
      * has not been called yet.
      */
     @CheckForNull
     public synchronized final Executor getExecutor() {
         return executor;
     }
 
+    /**
+     * Set the executor without notifying it about task completion.
+     * The caller <b>must</b> also call {@link #maybeComplete()}
+     * after releasing any problematic locks.
+     */
     @Restricted(NoExternalUse.class)
-    public synchronized final void setExecutor(@Nonnull Executor executor) {
-        assert this.executor==null;
-
+    public synchronized final void setExecutorWithoutCompleting(@Nonnull Executor executor) {
+        assert this.executor == null;
         this.executor = executor;
-        if (result!=null) {
-            executor.completedAsynchronous( result!=NULL ? result : null );
+    }
+
+    /**
+     * If there is a pending completion notification, deliver it to the executor.
+     * Must be called after {@link #setExecutorWithoutCompleting(Executor)}.
+     */
+    @Restricted(NoExternalUse.class)
+    public synchronized final void maybeComplete() {
+        assert this.executor != null;
+        if (result != null) {
+            executor.completedAsynchronous(result != NULL ? result : null);
             result = null;
         }
     }