[JIRA] [throttle-concurrent-builds-plugin] (JENKINS-27708) Concurrent build limits not honored on Jenkins 1.607

[stephenconno...@java.net (JIRA)]

stephenconnolly commented on JENKINS-27708 Concurrent build limits not honored on Jenkins 1.607 Here is one final version of a fix, presented for posterity - or if we need to revisit. diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index d776a57..67ba711 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -340,6 +340,25 @@ public class Queue extends ResourceController implements Saveable { private transient final Condition condition = lock.newCondition(); + /** + * There are some plugins that determine whether a project is blocked from execution on the basis of the + * jobs currently executing or currently pending execution. When we ask if a {@link Queue.Task} is blocked + * for execution from {@link #maintain()}, we need to ensure that the {@link Queue.Task#isBuildBlocked(Item)} + * is returning a result based on the live information of jobs scheduled rather than a result based on the + * most recent {@link #snapshot}. This field holds the identity of the current thread that is running + * {@link #maintain()} and thus we can switch to returning live results when called from that thread. + * + * Two alternative fixes for this class of issue (JENKINS-22708 and JENKINS-27871) could also be used: + * <ul> + * <li>Update the {@link #snapshot}</li> after each and every {@link Queue.Task} is assigned for execution.</li> + * <li>Have {@link #maintain()} return after finding one {@link Queue.Task} to execute.</li> + * </ul> + * @since 1.FIXME + * @see #liveResultRequired() + */ + @CheckForNull + private transient volatile Thread maintainThread = null; + public Queue(@Nonnull LoadBalancer loadBalancer) { this.loadBalancer = loadBalancer.sanitize(); // if all the executors are busy doing something, then the queue won't be maintained in @@ -844,6 +863,19 @@ public class Queue extends ResourceController implements Saveable { * Gets all the {@link BuildableItem}s that are waiting for an executor in the given {@link Computer}. */ public List<BuildableItem> getBuildableItems(Computer c) { + if (liveResultRequired() && lock.tryLock()) { + // JENKINS-22708 and JENKINS-27871. Note we should get the lock always as it should be already held + // but the safer code construct is to get the lock anyway rather than assume it is held just by virtue + // of the current thread being the thread running maintain(). + try { + List<BuildableItem> result = new ArrayList<BuildableItem>(); + _getBuildableItems(c, buildables, result); + _getBuildableItems(c, pendings, result); + return result; + } finally { + lock.unlock(); + } + } Snapshot snapshot = this.snapshot; List<BuildableItem> result = new ArrayList<BuildableItem>(); _getBuildableItems(c, snapshot.buildables, result); @@ -865,6 +897,18 @@ public class Queue extends ResourceController implements Saveable { * Gets the snapshot of all {@link BuildableItem}s. */ public List<BuildableItem> getBuildableItems() { + if (liveResultRequired() && lock.tryLock()) { + // JENKINS-22708 and JENKINS-27871. Note we should get the lock always as it should be already held + // but the safer code construct is to get the lock anyway rather than assume it is held just by virtue + // of the current thread being the thread running maintain(). + try { + ArrayList<BuildableItem> r = new ArrayList<BuildableItem>(buildables); + r.addAll(pendings); + return r; + } finally { + lock.unlock(); + } + } Snapshot snapshot = this.snapshot; ArrayList<BuildableItem> r = new ArrayList<BuildableItem>(snapshot.buildables); r.addAll(snapshot.pendings); @@ -875,6 +919,16 @@ public class Queue extends ResourceController implements Saveable { * Gets the snapshot of all {@link BuildableItem}s. */ public List<BuildableItem> getPendingItems() { + if (liveResultRequired() && lock.tryLock()) { + // JENKINS-22708 and JENKINS-27871. Note we should get the lock always as it should be already held + // but the safer code construct is to get the lock anyway rather than assume it is held just by virtue + // of the current thread being the thread running maintain(). + try { + return new ArrayList<BuildableItem>(pendings); + } finally { + lock.unlock(); + } + } return new ArrayList<BuildableItem>(snapshot.pendings); } @@ -902,6 +956,21 @@ public class Queue extends ResourceController implements Saveable { * @since 1.402 */ public List<Item> getUnblockedItems() { + if (liveResultRequired() && lock.tryLock()) { + // JENKINS-22708 and JENKINS-27871. Note we should get the lock always as it should be already held + // but the safer code construct is to get the lock anyway rather than assume it is held just by virtue + // of the current thread being the thread running maintain(). + try { + List<Item> queuedNotBlocked = new ArrayList<Item>(); + queuedNotBlocked.addAll(waitingList); + queuedNotBlocked.addAll(buildables); + queuedNotBlocked.addAll(pendings); + // but not 'blockedProjects' + return queuedNotBlocked; + } finally { + lock.unlock(); + } + } Snapshot snapshot = this.snapshot; List<Item> queuedNotBlocked = new ArrayList<Item>(); queuedNotBlocked.addAll(snapshot.waitingList); @@ -928,6 +997,19 @@ public class Queue extends ResourceController implements Saveable { * Is the given task currently pending execution? */ public boolean isPending(Task t) { + if (liveResultRequired() && lock.tryLock()) { + // JENKINS-22708 and JENKINS-27871. Note we should get the lock always as it should be already held + // but the safer code construct is to get the lock anyway rather than assume it is held just by virtue + // of the current thread being the thread running maintain(). + try { + for (BuildableItem i : pendings) + if (i.task.equals(t)) + return true; + return false; + } finally { + lock.unlock(); + } + } Snapshot snapshot = this.snapshot; for (BuildableItem i : snapshot.pendings) if (i.task.equals(t)) @@ -1250,6 +1332,18 @@ public class Queue extends ResourceController implements Saveable { } /** + * Some functionality that determines whether builds are blocked depending on what builds are currently running + * needs live information rather than information from the most recently updated snapshot. (See + * JENKINS-22708 and JENKINS-27871) + * @return {@code true} if the current thread is running {@link #maintain()} and thus live results should be + * returned rather than results from {@link #snapshot} + * @since 1.FIXME + */ + private boolean liveResultRequired() { + return Thread.currentThread() == maintainThread; + } + + /** * Queue maintenance. * * <p> @@ -1264,6 +1358,7 @@ public class Queue extends ResourceController implements Saveable { public void maintain() { lock.lock(); try { try { + maintainThread = Thread.currentThread(); LOGGER.log(Level.FINE, "Queue maintenance started {0}", this); @@ -1373,7 +1468,7 @@ public class Queue extends ResourceController implements Saveable { else LOGGER.log(Level.FINE, "BuildableItem {0} with empty work units!?", p); } - } finally { updateSnapshot(); } } finally { + } finally { maintainThread = null; updateSnapshot(); } } finally { lock.unlock(); } } I am going to apply Occam's razor and apply a slightly tweaked version of the first fix to core. Jobs are not scheduled for execution at a particularly onerous rate such that the creation of a new snapshot should not cause undue GC pressure, plus the snapshot itself should be easy for GC to clean up and should not really ever end up being promoted out of the eden pool, so simplest fix works

This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators. For more information on JIRA, see: http://www.atlassian.com/software/jira

--
You received this message because you are subscribed to the Google Groups "Jenkins Issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-issues+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.