[FIXED HUDSON-6819] Here is the race condition that induces this problem.

- Task X becomes buildable
- Thread E1 enters the pop() method, assigns X to an executor E2.
  Note that at this point X is not removed from the buildables list yet
- Thread E1 goes to offer.event.block and blocks, losing the lock
- Thread E3 enters the pop() method, assigns X to anogher executor E4.
  This is possible because X is still in the buildables list.
- Thread E3 goes to offer.event.block  and blocks, losing the lock.
- Thread E2 wakes up, obtains the lock and execute the rest of the
  pop() method. It decides that it's assigned X, it removes X from
  the buildables list.
- Thread E2 leaves the pop() method, loses the lock, and starts building X
- Thread E4  wakes up, obtains the lock, and executes the rest of the
  pop() method. It decides that it's assigned X. It tries to remove X
  from the buildables list. This remove operation becomes no-op,
  but we don't check that.
- Thread E4 leaves the pop(), and start building X.

Thus we end up building X twice.

To avoid this problem, I've introduced the 4th state "pending", and moves BuildableItem from buildables to pendings as soon as we assigned it to the executor. New BuildableItem cannot be created
while its another copy is either in the buildables or pendings state.

git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@32258 71c3de6d-444a-0410-be80-ed276b4c234a

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

@@ -105,7 +105,7 @@ import com.thoughtworks.xstream.converters.basic.AbstractSingleValueConverter;
  *                           |        ^
  *                           |        |
  *                           |        v
- *                           +--> buildables ---> (executed)
+ *                           +--> buildables ---> pending ---> (executed)
  * </pre>
  *
  * <p>
@@ -140,6 +140,11 @@ public class Queue extends ResourceController implements Saveable {
      */
     private final ItemList<BuildableItem> buildables = new ItemList<BuildableItem>();
 
+    /**
+     * {@link Task}s that are being handed over to the executor.
+     */
+    private final ItemList<BuildableItem> pendings = new ItemList<BuildableItem>();
+
     /**
      * Data structure created for each idle {@link Executor}.
      * This is a job offer from the queue to an executor.
@@ -541,7 +546,7 @@ public class Queue extends ResourceController implements Saveable {
     }
 
     public synchronized boolean isEmpty() {
-        return waitingList.isEmpty() && blockedProjects.isEmpty() && buildables.isEmpty();
+        return waitingList.isEmpty() && blockedProjects.isEmpty() && buildables.isEmpty() && pendings.isEmpty();
     }
 
     private synchronized WaitingItem peek() {
@@ -556,13 +561,15 @@ public class Queue extends ResourceController implements Saveable {
      */
     @Exported(inline=true)
     public synchronized Item[] getItems() {
-        Item[] r = new Item[waitingList.size() + blockedProjects.size() + buildables.size()];
+        Item[] r = new Item[waitingList.size() + blockedProjects.size() + buildables.size() + pendings.size()];
         waitingList.toArray(r);
         int idx = waitingList.size();
         for (BlockedItem p : blockedProjects.values())
             r[idx++] = p;
         for (BuildableItem p : reverse(buildables.values()))
             r[idx++] = p;
+        for (BuildableItem p : reverse(pendings.values()))
+            r[idx++] = p;
         return r;
     }
     
@@ -570,6 +577,7 @@ public class Queue extends ResourceController implements Saveable {
     	for (Item item: waitingList) if (item.id == id) return item;
     	for (Item item: blockedProjects) if (item.id == id) return item;
     	for (Item item: buildables) if (item.id == id) return item;
+        for (Item item: pendings) if (item.id == id) return item;
     	return null;
     }
 
@@ -578,7 +586,13 @@ public class Queue extends ResourceController implements Saveable {
      */
     public synchronized List<BuildableItem> getBuildableItems(Computer c) {
         List<BuildableItem> result = new ArrayList<BuildableItem>();
-        for (BuildableItem p : buildables.values()) {
+        _getBuildableItems(c, buildables, result);
+        _getBuildableItems(c, pendings, result);
+        return result;
+    }
+
+    private void _getBuildableItems(Computer c, ItemList<BuildableItem> col, List<BuildableItem> result) {
+        for (BuildableItem p : col.values()) {
             Label l = p.task.getAssignedLabel();
             if (l != null) {
                 // if a project has assigned label, it can be only built on it
@@ -587,14 +601,15 @@ public class Queue extends ResourceController implements Saveable {
             }
             result.add(p);
         }
-        return result;
     }
 
     /**
-     * Gets the snapshot of {@link #buildables}.
+     * Gets the snapshot of all {@link BuildableItem}s.
      */
     public synchronized List<BuildableItem> getBuildableItems() {
-        return new ArrayList<BuildableItem>(buildables.values());
+        ArrayList<BuildableItem> r = new ArrayList<BuildableItem>(buildables.values());
+        r.addAll(pendings.values());
+        return r;
     }
 
     /**
@@ -605,6 +620,9 @@ public class Queue extends ResourceController implements Saveable {
         for (BuildableItem bi : buildables.values())
             if(bi.task.getAssignedLabel()==l)
                 r++;
+        for (BuildableItem bi : pendings.values())
+            if(bi.task.getAssignedLabel()==l)
+                r++;
         return r;
     }
 
@@ -618,6 +636,9 @@ public class Queue extends ResourceController implements Saveable {
         if (bp!=null)
             return bp;
         BuildableItem bi = buildables.get(t);
+        if(bi!=null)
+            return bi;
+        bi = pendings.get(t);
         if(bi!=null)
             return bi;
 
@@ -637,6 +658,7 @@ public class Queue extends ResourceController implements Saveable {
     	List<Item> result =new ArrayList<Item>();
     	result.addAll(blockedProjects.getAll(t));
     	result.addAll(buildables.getAll(t));
+        result.addAll(pendings.getAll(t));
         for (Item item : waitingList) {
             if (item.task == t)
                 result.add(item);
@@ -657,7 +679,7 @@ public class Queue extends ResourceController implements Saveable {
      * Returns true if this queue contains the said project.
      */
     public synchronized boolean contains(Task t) {
-        if (blockedProjects.containsKey(t) || buildables.containsKey(t))
+        if (blockedProjects.containsKey(t) || buildables.containsKey(t) || pendings.containsKey(t))
             return true;
         for (Item item : waitingList) {
             if (item.task == t)
@@ -710,10 +732,9 @@ public class Queue extends ResourceController implements Saveable {
                     assert runner.canTake(p.task);
 
                     // found a matching executor. use it.
-                    // the item is retracted from the buildables list by the the executor that'll be actually
-                    // running the job, so that the state change from "buildable item" to "building" happens
-                    // atomically.
                     runner.set(p);
+                    itr.remove();
+                    pendings.add(p);
                 }
 
                 // we went over all the buildable projects and awaken
@@ -741,7 +762,7 @@ public class Queue extends ResourceController implements Saveable {
                     // if so, just build it
                     LOGGER.fine("Pop returning " + offer.item + " for " + exec.getName());
                     offer.item.future.startExecuting(exec);
-                    buildables.remove(offer.item);
+                    pendings.remove(offer.item);
                     return offer.item;
                 }
                 // otherwise run a queue maintenance
@@ -852,8 +873,8 @@ public class Queue extends ResourceController implements Saveable {
     }
 
     /**
-     *  Make sure we don't queue two tasks of the same project to be built
-     *  unless that project allows concurrent builds.
+     * Make sure we don't queue two tasks of the same project to be built
+     * unless that project allows concurrent builds.
      */
     private boolean allowNewBuildableTask(Task t) {
         try {
@@ -862,7 +883,7 @@ public class Queue extends ResourceController implements Saveable {
         } catch (AbstractMethodError e) {
             // earlier versions don't have the "isConcurrentBuild" method, so fall back gracefully
         }
-        return !buildables.containsKey(t);
+        return !buildables.containsKey(t) && !pendings.containsKey(t);
     }
 
     /**