[JENKINS-26323] Fix "Build Current Patches Only" by scanning for event

The "Build Current Patches Only" feature currently identifies previous builds based on their parameters as they were present when the build was first scheduled. If you use a plugin that alters the parameters after the job has started, the cancelling of previous builds does not work, since the GerritTrigger#cancelJob() method does not find the job anymore. This is easily and trivially fixed, by not scanning for the parameters, but instead for the GerritEvent that is stored in the GerritCause used to start a build. The only downside to this is, that the current patch-set scans for identity, but not equality of the two events. After a Jenkins restart, they might not be identical anymore, since Jenkins reschedules queued builds and can't unify the two events via Stapler; as they are stored in separate files. It'd be trivially easy to alter this to Event.equals(otherEvent), in case this degradation is not wanted.
jenkinsci · Jan 7, 2015 · 97b08ee · 97b08ee
1 parent c3899ea
commit 97b08ee
Showing 1 changed file with 50 additions and 21 deletions.
diff --git a/...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java b/...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
@@ -71,6 +71,7 @@
 import hudson.model.Action;
 import hudson.model.Actionable;
 import hudson.model.AutoCompletionCandidates;
+import hudson.model.Cause;
 import hudson.model.Computer;
 import hudson.model.Executor;
 import hudson.model.Hudson;
@@ -79,6 +80,7 @@
 import hudson.model.Job;
 import hudson.model.ParametersAction;
 import hudson.model.Queue;
+import hudson.model.Run;
 import hudson.model.Result;
 import hudson.triggers.Trigger;
 import hudson.triggers.TriggerDescriptor;
@@ -94,6 +96,7 @@
 import java.text.MessageFormat;
 import java.text.ParseException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -1835,7 +1838,7 @@ public synchronized void scheduled(ChangeBasedEvent event, ParametersAction para
                     if (((ChangeBasedEvent)pairs.getKey()).getChange().equals(event.getChange())) {
                         logger.debug("Cancelling build for " + pairs.getKey());
                         try {
-                            cancelJob(pairs.getValue());
+                            cancelJob(pairs.getKey());
                         } catch (Exception e) {
                             // Ignore any problems with canceling the job.
                             logger.error("Error canceling job", e);
@@ -1849,23 +1852,27 @@ public synchronized void scheduled(ChangeBasedEvent event, ParametersAction para
         }
 
         /**
-         * Tries to cancel any jobs with the specified parameters. We look in
-         * both the build queue and currently executing jobs. This extra work is
-         * required due to race conditions when calling Future.cancel() - see
+         * Tries to cancel any job, which was triggered by the given change event.
+         * <p>
+         * Since the event is always noted in the build cause, it is easy to
+         * identify which specific builds shall be cancelled, without having
+         * to dig down into the parameters, which might've been mutated by the
+         * build while it was running. (This was the previous implementation)
+         * <p>
+         * We look in both the build queue and currently executing jobs.
+         * This extra work is required due to race conditions when calling
+         * Future.cancel() - see
          * https://issues.jenkins-ci.org/browse/JENKINS-13829
          *
-         * @param parameters
-         *            The parameters to match against.
+         * @param event
+         *            The event that originally triggered the build.
          */
-        private void cancelJob(ParametersAction parameters) {
+        private void cancelJob(GerritTriggeredEvent event) {
             // Remove any jobs in the build queue.
-            List<hudson.model.Queue.Item> itemsInQueue = Queue.getInstance().getItems(job);
-            for (hudson.model.Queue.Item item  : itemsInQueue) {
-                List<ParametersAction> params = item.getActions(ParametersAction.class);
-                for (ParametersAction param : params) {
-                    if (param.equals(parameters)) {
-                        Queue.getInstance().cancel(item);
-                    }
+            List<hudson.model.Queue.Item> itemsInQueue = Queue.getInstance().getItems(myProject);
+            for (hudson.model.Queue.Item item : itemsInQueue) {
+                if (checkCausedByGerrit(event, item.getCauses())) {
+                    Queue.getInstance().cancel(item);
                 }
             }
             // Interrupt any currently running jobs.
@@ -1874,18 +1881,40 @@ private void cancelJob(ParametersAction parameters) {
                 executors.addAll(c.getOneOffExecutors());
                 executors.addAll(c.getExecutors());
                 for (Executor e : executors) {
-                    if (e.getCurrentExecutable() instanceof Actionable) {
-                        Actionable a = (Actionable)e.getCurrentExecutable();
-                        List<ParametersAction> params = a.getActions(ParametersAction.class);
-                        for (ParametersAction param : params) {
-                            if (param.equals(parameters)) {
-                                e.interrupt(Result.ABORTED, new NewPatchSetInterruption());
-                            }
+                    if (e.getCurrentExecutable() instanceof Run<?,?>) {
+                        Run<?,?> run = (Run<?,?>) e.getCurrentExecutable();
+                        if (checkCausedByGerrit(event, run.getCauses())) {
+                            e.interrupt(
+                                    Result.ABORTED,
+                                    new NewPatchSetInterruption()
+                            );
                         }
                     }
                 }
             }
         }
+
+        /**
+         * Checks if any of the given causes references the given event.
+         * 
+         * @param event The event to check for. Checks for <i>identity</i>, not
+         * <i>equality</i>!
+         * @param causes the list of causes. Only {@link GerritCause}s are considered.
+         * @return
+         */
+        private boolean checkCausedByGerrit(GerritTriggeredEvent event, Collection<Cause> causes) {
+            for (Cause c : causes) {
+                if (!(c instanceof GerritCause)) {
+                    continue;
+                }
+                GerritCause gc = (GerritCause) c;
+                if (gc.getEvent() == event) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
 
         /**
          * Removes any reference to the current build for this change.