[FIXED JENKINS-10944] [FIXED JENKINS-24519] If makeBuildable fails on a...

[FIXED JENKINS-10944] [FIXED JENKINS-24519] If makeBuildable fails on a FlyweightTask, keep it in queue.

core/src/main/java/hudson/model/Queue.java

@@ -106,7 +106,6 @@ import javax.servlet.ServletException;
 
 import jenkins.model.Jenkins;
 import jenkins.security.QueueItemAuthenticator;
-import jenkins.security.QueueItemAuthenticatorConfiguration;
 import jenkins.util.AtmostOneTaskExecutor;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
@@ -1098,8 +1097,11 @@ public class Queue extends ResourceController implements Saveable {
             for (BlockedItem p : new ArrayList<BlockedItem>(blockedProjects.values())) {// copy as we'll mutate the list
                 if (!isBuildBlocked(p) && allowNewBuildableTask(p.task)) {
                     // ready to be executed
-                    p.leave(this);
-                    makeBuildable(new BuildableItem(p));
+                    Runnable r = makeBuildable(new BuildableItem(p));
+                    if (r != null) {
+                        p.leave(this);
+                        r.run();
+                    }
                 }
             }
         }
@@ -1115,7 +1117,12 @@ public class Queue extends ResourceController implements Saveable {
             Task p = top.task;
             if (!isBuildBlocked(top) && allowNewBuildableTask(p)) {
                 // ready to be executed immediately
-                makeBuildable(new BuildableItem(top));
+                Runnable r = makeBuildable(new BuildableItem(top));
+                if (r != null) {
+                    r.run();
+                } else {
+                    new BlockedItem(top).enter(this);
+                }
             } else {
                 // this can't be built now because another build is in progress
                 // set this project aside.
@@ -1164,10 +1171,14 @@ public class Queue extends ResourceController implements Saveable {
         }
     }
 
-    private void makeBuildable(BuildableItem p) {
-        if (p.task instanceof FlyweightTask && !isBlockedByShutdown(p.task)) {
-
-
+    /**
+     * Tries to make an item ready to build.
+     * @param p a proposed buildable item
+     * @return a thunk to actually prepare it (after leaving an earlier list), or null if it cannot be run now
+     */
+    private @CheckForNull Runnable makeBuildable(final BuildableItem p) {
+        if (p.task instanceof FlyweightTask) {
+            if (!isBlockedByShutdown(p.task)) {
             Jenkins h = Jenkins.getInstance();
             Map<Node,Integer> hashSource = new HashMap<Node, Integer>(h.getNodes().size());
 
@@ -1183,19 +1194,27 @@ public class Queue extends ResourceController implements Saveable {
 
             Label lbl = p.getAssignedLabel();
             for (Node n : hash.list(p.task.getFullDisplayName())) {
-                Computer c = n.toComputer();
+                final Computer c = n.toComputer();
                 if (c==null || c.isOffline())    continue;
                 if (lbl!=null && !lbl.contains(n))  continue;
                 if (n.canTake(p) != null) continue;
-                c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task));
-                makePending(p);
-                return;
+                return new Runnable() {
+                    @Override public void run() {
+                        c.startFlyWeightTask(new WorkUnitContext(p).createWorkUnit(p.task));
+                        makePending(p);
+                    }
+                };
+            }
             }
             // if the execution get here, it means we couldn't schedule it anywhere.
-            // so do the scheduling like other normal jobs.
+            return null;
+        } else { // regular heavyweight task
+            return new Runnable() {
+                @Override public void run() {
+                    p.enter(Queue.this);
+                }
+            };
         }
-
-        p.enter(this);
     }
 
 

test/src/test/java/hudson/model/QueueTest.java

@@ -345,6 +345,49 @@ public class QueueTest {
         assertEquals(1, runs.size());
         assertEquals("slave0", runs.get(0).getBuiltOnStr());
     }
+    
+    @Issue("JENKINS-10944")
+    @Test public void flyweightTasksBlockedByShutdown() throws Exception {
+        r.jenkins.doQuietDown(true, 0);
+        AtomicInteger cnt = new AtomicInteger();
+        TestFlyweightTask task = new TestFlyweightTask(cnt, null);
+        assertTrue(Queue.isBlockedByShutdown(task));
+        r.jenkins.getQueue().schedule2(task, 0);
+        r.jenkins.getQueue().maintain();
+        r.jenkins.doCancelQuietDown();
+        assertFalse(Queue.isBlockedByShutdown(task));
+        r.waitUntilNoActivity();
+        assertEquals(1, cnt.get());
+        assert task.exec instanceof OneOffExecutor : task.exec;
+    }
+
+    @Issue("JENKINS-24519")
+    @Test public void flyweightTasksBlockedBySlave() throws Exception {
+        Label label = Label.get("myslave");
+        AtomicInteger cnt = new AtomicInteger();
+        TestFlyweightTask task = new TestFlyweightTask(cnt, label);
+        r.jenkins.getQueue().schedule2(task, 0);
+        r.jenkins.getQueue().maintain();
+        r.createSlave(label);
+        r.waitUntilNoActivity();
+        assertEquals(1, cnt.get());
+        assert task.exec instanceof OneOffExecutor : task.exec;
+    }
+
+    private static class TestFlyweightTask extends TestTask implements Queue.FlyweightTask {
+        Executor exec;
+        private final Label assignedLabel;
+        TestFlyweightTask(AtomicInteger cnt, Label assignedLabel) {
+            super(cnt);
+            this.assignedLabel = assignedLabel;
+        }
+        @Override protected void doRun() {
+            exec = Executor.currentExecutor();
+        }
+        @Override public Label getAssignedLabel() {
+            return assignedLabel;
+        }
+    }
 
     @Test public void taskEquality() throws Exception {
         AtomicInteger cnt = new AtomicInteger();
@@ -356,7 +399,7 @@ public class QueueTest {
         r.waitUntilNoActivity();
         assertEquals(1, cnt.get());
     }
-    private static final class TestTask extends AbstractQueueTask {
+    private static class TestTask extends AbstractQueueTask {
         private final AtomicInteger cnt;
         TestTask(AtomicInteger cnt) {
             this.cnt = cnt;
@@ -379,11 +422,13 @@ public class QueueTest {
         @Override public Node getLastBuiltOn() {return null;}
         @Override public long getEstimatedDuration() {return -1;}
         @Override public ResourceList getResourceList() {return new ResourceList();}
+        protected void doRun() {}
         @Override public Executable createExecutable() throws IOException {
             return new Executable() {
                 @Override public SubTask getParent() {return TestTask.this;}
                 @Override public long getEstimatedDuration() {return -1;}
                 @Override public void run() {
+                    doRun();
                     cnt.incrementAndGet();
                 }
             };