Merge pull request #1743 from stephenc/jenkins-28926

[FIXED JENKINS-28926] Block while upstream/downstream building cycles never complete
jenkinsci · Jun 17, 2015 · 7929412 · 7929412
2 parents 482bffa + c44c088
commit 7929412
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 2 deletions.
diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java
@@ -1408,15 +1408,29 @@ public void maintain() {
                 }
             }
 
+            final QueueSorter s = sorter;
 
             {// blocked -> buildable
-                for (BlockedItem p : new ArrayList<BlockedItem>(blockedProjects.values())) {// copy as we'll mutate the list
+                // copy as we'll mutate the list and we want to process in a potentially different order
+                // TODO replace with <> operator after backporting to 1.609.3
+                List<BlockedItem> blockedItems = new ArrayList<BlockedItem>(blockedProjects.values());
+                // if facing a cycle of blocked tasks, ensure we process in the desired sort order
+                if (s != null) {
+                    s.sortBlockedItems(blockedItems);
+                } else {
+                    Collections.sort(blockedItems, QueueSorter.DEFAULT_BLOCKED_ITEM_COMPARATOR);
+                }
+                for (BlockedItem p : blockedItems) {
                     if (!isBuildBlocked(p) && allowNewBuildableTask(p.task)) {
                         // ready to be executed
                         Runnable r = makeBuildable(new BuildableItem(p));
                         if (r != null) {
                             p.leave(this);
                             r.run();
+                            // JENKINS-28926 we have removed a task from the blocked projects and added to building
+                            // thus we should update the snapshot so that subsequent blocked projects can correctly
+                            // determine if they are blocked by the lucky winner
+                            updateSnapshot();
                         }
                     }
                 }
@@ -1446,7 +1460,6 @@ public void maintain() {
                 }
             }
 
-            final QueueSorter s = sorter;
             if (s != null)
                 s.sortBuildableItems(buildables);
 
@@ -1461,6 +1474,9 @@ public void maintain() {
                     p.leave(this);
                     new BlockedItem(p).enter(this);
                     LOGGER.log(Level.FINE, "Catching that {0} is blocked in the last minute", p);
+                    // JENKINS-28926 we have moved an unblocked task into the blocked state, update snapshot
+                    // so that other buildables which might have been blocked by this can see the state change
+                    updateSnapshot();
                     continue;
                 }
 

diff --git a/core/src/main/java/hudson/model/queue/QueueSorter.java b/core/src/main/java/hudson/model/queue/QueueSorter.java
@@ -8,6 +8,8 @@
 import hudson.model.Queue;
 import hudson.model.Queue.BuildableItem;
 
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.logging.Logger;
 
@@ -19,6 +21,20 @@
  * @since 1.343
  */
 public abstract class QueueSorter implements ExtensionPoint {
+    /**
+     * A comparator that sorts {@link Queue.BlockedItem} instances based on how long they have been in the queue.
+     * (We want the time since in queue by default as blocking on upstream/downstream considers waiting items
+     * also and thus the blocking starts once the task is in the queue not once the task is buildable)
+     *
+     * @since 1.FIXME
+     */
+    public static final Comparator<Queue.BlockedItem> DEFAULT_BLOCKED_ITEM_COMPARATOR = new Comparator<Queue.BlockedItem>() {
+        @Override
+        public int compare(Queue.BlockedItem o1, Queue.BlockedItem o2) {
+            return Long.compare(o1.getInQueueSince(), o2.getInQueueSince());
+        }
+    };
+
     /**
      * Sorts the buildable items list. The items at the beginning will be executed
      * before the items at the end of the list.
@@ -28,6 +44,18 @@ public abstract class QueueSorter implements ExtensionPoint {
      */
     public abstract void sortBuildableItems(List<BuildableItem> buildables);
 
+    /**
+     * Sorts the blocked items list. The items at the beginning will be considered for removal from the blocked state
+     * before the items at the end of the list.
+     *
+     * @param blockedItems
+     *      List of blocked items in the queue. Never null.
+     * @since 1.FIXME
+     */
+    public void sortBlockedItems(List<Queue.BlockedItem> blockedItems) {
+        Collections.sort(blockedItems, DEFAULT_BLOCKED_ITEM_COMPARATOR);
+    }
+
     /**
      * All registered {@link QueueSorter}s. Only the first one will be picked up,
      * unless explicitly overridden by {@link Queue#setSorter(QueueSorter)}.

diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java
@@ -75,6 +75,7 @@
 import javax.servlet.http.HttpServletResponse;
 import jenkins.model.Jenkins;
 import jenkins.security.QueueItemAuthenticatorConfiguration;
+import jenkins.triggers.ReverseBuildTrigger;
 import org.acegisecurity.Authentication;
 import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.acls.sid.PrincipalSid;
@@ -83,6 +84,9 @@
 import org.apache.commons.fileupload.disk.DiskFileItemFactory;
 import org.apache.commons.fileupload.servlet.ServletFileUpload;
 import org.apache.commons.io.FileUtils;
+
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.*;
 
 import org.junit.Assert;
@@ -442,6 +446,38 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         String tagName = queueItem.getDocumentElement().getTagName();
         assertTrue(tagName.equals("blockedItem") || tagName.equals("buildableItem"));
     }
+
+    @Issue("JENKINS-28926")
+    @Test
+    public void upstreamDownstreamCycle() throws Exception {
+        FreeStyleProject trigger = r.createFreeStyleProject();
+        FreeStyleProject chain1 = r.createFreeStyleProject();
+        FreeStyleProject chain2a = r.createFreeStyleProject();
+        FreeStyleProject chain2b = r.createFreeStyleProject();
+        FreeStyleProject chain3 = r.createFreeStyleProject();
+        trigger.getPublishersList().add(new BuildTrigger(String.format("%s, %s, %s, %s", chain1.getName(), chain2a.getName(), chain2b.getName(), chain3.getName()), true));
+        trigger.setQuietPeriod(0);
+        chain1.setQuietPeriod(1);
+        chain2a.setQuietPeriod(1);
+        chain2b.setQuietPeriod(1);
+        chain3.setQuietPeriod(1);
+        chain1.getPublishersList().add(new BuildTrigger(String.format("%s, %s", chain2a.getName(), chain2b.getName()), true));
+        chain2a.getPublishersList().add(new BuildTrigger(chain3.getName(), true));
+        chain2b.getPublishersList().add(new BuildTrigger(chain3.getName(), true));
+        chain1.setBlockBuildWhenDownstreamBuilding(true);
+        chain2a.setBlockBuildWhenDownstreamBuilding(true);
+        chain2b.setBlockBuildWhenDownstreamBuilding(true);
+        chain3.setBlockBuildWhenUpstreamBuilding(true);
+        r.jenkins.rebuildDependencyGraph();
+        r.buildAndAssertSuccess(trigger);
+        // the trigger should build immediately and schedule the cycle
+        r.waitUntilNoActivity();
+        final Queue queue = r.getInstance().getQueue();
+        assertThat("The cycle should have been defanged and chain1 executed", queue.getItem(chain1), nullValue());
+        assertThat("The cycle should have been defanged and chain2a executed", queue.getItem(chain2a), nullValue());
+        assertThat("The cycle should have been defanged and chain2b executed", queue.getItem(chain2b), nullValue());
+        assertThat("The cycle should have been defanged and chain3 executed", queue.getItem(chain3), nullValue());
+    }
 
     private static class TestFlyweightTask extends TestTask implements Queue.FlyweightTask {
         Executor exec;