[JENKINS-26761] Merging #160.

jenkinsci · Jul 29, 2015 · f6f21ae · f6f21ae
2 parents 121889f + ad9b6ca
commit f6f21ae
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 14 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -4,6 +4,8 @@ Only noting significant user-visible or major API changes, not internal code cle
 
 ## 1.9 (upcoming)
 
+* [JENKINS-26761](https://issues.jenkins-ci.org/browse/JENKINS-26761): `NullPointerException` from Git commit notification requests under unknown circumstances; improved robustness and logging.
+
 ## 1.9-beta-1 (Jul 27 2015)
 
 * [JENKINS-26129](https://issues.jenkins-ci.org/browse/JENKINS-26129): Experimental support for multibranch workflows. (For now, in a separate plugin, not included in _Workflow: Aggregator_, since it depends on the Branch API plugin which does not have a non-beta release and so is available only from the experimental update center.)

diff --git a/aggregator/src/test/java/org/jenkinsci/plugins/workflow/steps/scm/GitStepRestartTest.java b/aggregator/src/test/java/org/jenkinsci/plugins/workflow/steps/scm/GitStepRestartTest.java
@@ -27,20 +27,25 @@
 import hudson.model.Label;
 import hudson.scm.ChangeLogSet;
 import hudson.triggers.SCMTrigger;
+import java.io.File;
 import java.util.Iterator;
 import java.util.List;
+import org.apache.commons.io.FileUtils;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.junit.Test;
 import static org.junit.Assert.*;
+import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
 
 public class GitStepRestartTest {
 
+    @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public RestartableJenkinsRule r = new RestartableJenkinsRule();
     @Rule public GitSampleRepoRule sampleRepo = new GitSampleRepoRule();
 
@@ -61,6 +66,7 @@ public class GitStepRestartTest {
                 p.save();
                 WorkflowRun b = r.j.assertBuildStatusSuccess(p.scheduleBuild2(0));
                 r.j.assertLogContains("Cloning the remote Git repository", b);
+                FileUtils.copyFile(new File(b.getRootDir(), "build.xml"), System.out);
             }
         });
         r.addStep(new Statement() {

diff --git a/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowJob.java b/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowJob.java
@@ -439,7 +439,7 @@ public void addTrigger(Trigger trigger) {
             return Collections.emptySet();
         }
         List<SCM> scms = new LinkedList<SCM>();
-        for (WorkflowRun.SCMCheckout co : b.checkouts) {
+        for (WorkflowRun.SCMCheckout co : b.checkouts(null)) {
             scms.add(co.scm);
         }
         return scms;
@@ -464,7 +464,7 @@ public void addTrigger(Trigger trigger) {
             listener.getLogger().println("no previous build to compare to");
             return PollingResult.NO_CHANGES;
         }
-        for (WorkflowRun.SCMCheckout co : b.checkouts) {
+        for (WorkflowRun.SCMCheckout co : b.checkouts(listener)) {
             if (!co.scm.supportsPolling()) {
                 listener.getLogger().println("polling not supported from " + co.workspace + " on " + co.node);
             }

diff --git a/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
@@ -30,6 +30,7 @@
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionList;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.AbortException;
 import hudson.Extension;
 import hudson.FilePath;
@@ -50,6 +51,7 @@
 import hudson.scm.SCM;
 import hudson.scm.SCMRevisionState;
 import hudson.util.NullStream;
+import hudson.util.PersistedList;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
@@ -61,7 +63,6 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Future;
@@ -95,7 +96,7 @@
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
 @SuppressWarnings("SynchronizeOnNonFinalField")
-@edu.umd.cs.findbugs.annotations.SuppressWarnings("JLM_JSR166_UTILCONCURRENT_MONITORENTER") // completed is an unusual usage
+@SuppressFBWarnings(value="JLM_JSR166_UTILCONCURRENT_MONITORENTER", justification="completed is an unusual usage")
 public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements Queue.Executable, LazyBuildMixIn.LazyLoadingRun<WorkflowJob,WorkflowRun> {
 
     private static final Logger LOGGER = Logger.getLogger(WorkflowRun.class.getName());
@@ -118,7 +119,8 @@ public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements Q
     /** map from node IDs to log positions from which we should copy text */
     private Map<String,Long> logsToCopy;
 
-    List<SCMCheckout> checkouts;
+    /** JENKINS-26761: supposed to always be set but sometimes is not. Access only through {@link #checkouts(TaskListener)}. */
+    private @CheckForNull List<SCMCheckout> checkouts;
     // TODO could use a WeakReference to reduce memory, but that complicates how we add to it incrementally; perhaps keep a List<WeakReference<ChangeLogSet<?>>>
     private transient List<ChangeLogSet<? extends ChangeLogSet.Entry>> changeSets;
 
@@ -128,6 +130,7 @@ public final class WorkflowRun extends Run<WorkflowJob,WorkflowRun> implements Q
     public WorkflowRun(WorkflowJob job) throws IOException {
         super(job);
         firstTime = true;
+        checkouts = new PersistedList<SCMCheckout>(this);
         //System.err.printf("created %s @%h%n", this, this);
     }
 
@@ -178,7 +181,6 @@ public WorkflowRun(WorkflowJob job, File dir) throws IOException {
                 listener.error("No flow definition, cannot run");
                 return;
             }
-            checkouts = new LinkedList<SCMCheckout>();
             Owner owner = new Owner(this);
 
             FlowExecution newExecution = definition.create(owner, listener, getAllActions());
@@ -443,6 +445,7 @@ private String key() {
                 Queue.getInstance().schedule(new AfterRestartTask(this), 0);
             }
         }
+        checkouts(null); // only for diagnostics
         synchronized (LOADING_RUNS) {
             LOADING_RUNS.remove(key()); // or could just make the value type be WeakReference<WorkflowRun>
             LOADING_RUNS.notifyAll();
@@ -522,15 +525,23 @@ public boolean hasntStartedYet() {
         return isBuilding(); // there is no equivalent to a post-production state for flows
     }
 
+    synchronized @Nonnull List<SCMCheckout> checkouts(@CheckForNull TaskListener listener) {
+        if (checkouts == null) {
+            LOGGER.log(Level.WARNING, "JENKINS-26761: no checkouts in {0}", this);
+            if (listener != null) {
+                listener.error("JENKINS-26761: list of SCM checkouts in " + this + " was lost; polling will be broken");
+            }
+            checkouts = new PersistedList<SCMCheckout>(this);
+            // Could this.save(), but might pollute diagnosis, and (worse) might clobber real data if there is >1 WorkflowRun with the same ID.
+        }
+        return checkouts;
+    }
+
     @Exported
     public synchronized List<ChangeLogSet<? extends ChangeLogSet.Entry>> getChangeSets() {
         if (changeSets == null) {
             changeSets = new ArrayList<ChangeLogSet<? extends ChangeLogSet.Entry>>();
-            if (checkouts == null) {
-                LOGGER.log(Level.WARNING, "no checkouts in {0}", this);
-                return changeSets;
-            }
-            for (SCMCheckout co : checkouts) {
+            for (SCMCheckout co : checkouts(null)) {
                 if (co.changelogFile != null && co.changelogFile.isFile()) {
                     try {
                         changeSets.add(co.scm.createChangeLogParser().parse(this, co.scm.getEffectiveBrowser(), co.changelogFile));
@@ -553,7 +564,7 @@ public synchronized HttpResponse doStop() {
         }
     }
 
-    private void onCheckout(SCM scm, FilePath workspace, @CheckForNull File changelogFile, @CheckForNull SCMRevisionState pollingBaseline) throws Exception {
+    private void onCheckout(SCM scm, FilePath workspace, TaskListener listener, @CheckForNull File changelogFile, @CheckForNull SCMRevisionState pollingBaseline) throws Exception {
         if (changelogFile != null && changelogFile.isFile()) {
             ChangeLogSet<?> cls = scm.createChangeLogParser().parse(this, scm.getEffectiveBrowser(), changelogFile);
             getChangeSets().add(cls);
@@ -566,7 +577,7 @@ private void onCheckout(SCM scm, FilePath workspace, @CheckForNull File changelo
         if (computer == null) {
             throw new IllegalStateException();
         }
-        checkouts.add(new SCMCheckout(scm, computer.getName(), workspace.getRemote(), changelogFile, pollingBaseline));
+        checkouts(listener).add(new SCMCheckout(scm, computer.getName(), workspace.getRemote(), changelogFile, pollingBaseline));
     }
 
     static final class SCMCheckout {
@@ -716,7 +727,7 @@ static void alias() {
     @Extension public static final class SCMListenerImpl extends SCMListener {
         @Override public void onCheckout(Run<?,?> build, SCM scm, FilePath workspace, TaskListener listener, File changelogFile, SCMRevisionState pollingBaseline) throws Exception {
             if (build instanceof WorkflowRun) {
-                ((WorkflowRun) build).onCheckout(scm, workspace, changelogFile, pollingBaseline);
+                ((WorkflowRun) build).onCheckout(scm, workspace, listener, changelogFile, pollingBaseline);
             }
         }
     }