[FIXED JENKINS-12318]

Preserve in-progress builds when reloading a job. Because in-flight builds tend to update the state a lot, with this change we refrain from reloading those builds from the disk. This should be acceptable since we are primarily reloading a job, and reloading of builds are secondary.
jenkinsci · Jun 19, 2012 · 9fbd6d3 · 9fbd6d3
1 parent aa5e079
commit 9fbd6d3
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 5 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,6 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    Updating job config.xml shouldn't clobber in-progress builds.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-12318">issue 12318</a>)
   <li class=rfe>
     Search index includes all top-level jobs, not just jobs in the current view.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-13148">issue 13148</a>)

diff --git a/core/src/main/java/hudson/cli/UpdateJobCommand.java b/core/src/main/java/hudson/cli/UpdateJobCommand.java
@@ -27,6 +27,7 @@
 import hudson.model.AbstractProject;
 import org.kohsuke.args4j.Argument;
 
+import javax.xml.transform.Source;
 import javax.xml.transform.stream.StreamSource;
 
 /**
@@ -43,7 +44,7 @@ public String getShortDescription() {
     }
 
     protected int run() throws Exception {
-        job.updateByXml(new StreamSource(stdin));
+        job.updateByXml((Source)new StreamSource(stdin));
         return 0;
     }
 }

diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
@@ -65,6 +65,7 @@
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.servlet.ServletException;
+import javax.xml.transform.Source;
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.TransformerFactory;
@@ -546,7 +547,7 @@ public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp)
         }
         if (req.getMethod().equals("POST")) {
             // submission
-            updateByXml(new StreamSource(req.getReader()));
+            updateByXml((Source)new StreamSource(req.getReader()));
             return;
         }
 
@@ -555,9 +556,17 @@ public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp)
     }
 
     /**
-     * Updates Job by its XML definition.
+     * @deprecated as of 1.473
+     *      Use {@link #updateByXml(Source)}
      */
     public void updateByXml(StreamSource source) throws IOException {
+        updateByXml((Source)source);
+    }
+
+    /**
+     * Updates Job by its XML definition.
+     */
+    public void updateByXml(Source source) throws IOException {
         checkPermission(CONFIGURE);
         XmlFile configXmlFile = getConfigFile();
         AtomicFileWriter out = new AtomicFileWriter(configXmlFile.getFile());

diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java
@@ -273,7 +273,8 @@ public void onCreatedFromScratch() {
     public void onLoad(ItemGroup<? extends Item> parent, String name) throws IOException {
         super.onLoad(parent, name);
 
-        this.builds = new RunMap<R>();
+        if (this.builds==null)
+            this.builds = new RunMap<R>();
         this.builds.load(this,new Constructor<R>() {
             public R create(File dir) throws IOException {
                 return loadBuild(dir);

diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java
@@ -23,6 +23,8 @@
  */
 package hudson.model;
 
+import com.google.common.collect.Maps;
+
 import java.io.File;
 import java.io.FilenameFilter;
 import java.io.IOException;
@@ -163,6 +165,24 @@ public Integer lastKey() {
         return builds.lastKey();
     }
 
+    /**
+     * This is the newest build (with the biggest build number)
+     */
+    public R newestValue() {
+        SortedMap<Integer, R> _builds = builds;
+        if (_builds.isEmpty())  return null;
+        return _builds.get(firstKey());
+    }
+
+    /**
+     * This is the oldest build (with the smallest build number)
+     */
+    public R oldestValue() {
+        SortedMap<Integer, R> _builds = builds;
+        if (_builds.isEmpty())  return null;
+        return _builds.get(lastKey());
+    }
+
     public static final Comparator<Comparable> COMPARATOR = new Comparator<Comparable>() {
         public int compare(Comparable o1, Comparable o2) {
             return -o1.compareTo(o2);
@@ -192,7 +212,7 @@ public synchronized void load(Job job, Constructor<R> cons) {
         buildDir.mkdirs();
         String[] buildDirs = buildDir.list(new FilenameFilter() {
             public boolean accept(File dir, String name) {
-                // HUDSON-1461 sometimes create bogus data directories with impossible dates, such as year 0, April 31st,
+                // JENKINS-1461 sometimes create bogus data directories with impossible dates, such as year 0, April 31st,
                 // or August 0th. Date object doesn't roundtrip those, so we eventually fail to load this data.
                 // Don't even bother trying.
                 if (!isCorrectDate(name)) {
@@ -213,6 +233,12 @@ private boolean isCorrectDate(String name) {
             }
         });
 
+        // keep all those that are building intact.
+        Map<Integer,R> building = Maps.newHashMap();
+        for (R b=newestValue(); b!=null && b.isBuilding(); b=b.getPreviousBuild()) {
+            building.put(b.getNumber(), b);
+        }
+
         for( String build : buildDirs ) {
             File d = new File(buildDir,build);
             if(new File(d,"build.xml").exists()) {
@@ -228,6 +254,22 @@ private boolean isCorrectDate(String name) {
             }
         }
 
+        // overlay what's currently building on top of what's loaded
+        builds.putAll(building);
+        if (false) {
+            // we probably aren't saving every little changes during the build to disk,
+            // so it's risky to reload these from disk.
+            for (R b : building.values()) {
+                try {
+                    b.reload();
+                } catch (IOException e) {
+                    e.printStackTrace();
+                } catch (InstantiationError e) {
+                    e.printStackTrace();
+                }
+            }
+        }
+
         reset(builds);
 
         for (R r : builds.values())

diff --git a/test/src/test/groovy/hudson/model/RunMapTest.groovy b/test/src/test/groovy/hudson/model/RunMapTest.groovy
@@ -0,0 +1,43 @@
+package hudson.model
+
+import org.jvnet.hudson.test.Bug
+import org.jvnet.hudson.test.HudsonTestCase
+import org.jvnet.hudson.test.SleepBuilder
+
+import javax.xml.transform.stream.StreamSource
+
+/**
+ *
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class RunMapTest extends HudsonTestCase {
+    /**
+     * Makes sure that reloading the project while a build is in progress won't clobber that in-progress build.
+     */
+    @Bug(12318)
+    public void testReloadWhileBuildIsInProgress() {
+        def p = createFreeStyleProject()
+
+        // want some completed build records
+        def b1 = assertBuildStatusSuccess(p.scheduleBuild2(0))
+
+        // now create a build that hangs until we signal the OneShotEvent
+        p.buildersList.add(new SleepBuilder(9999999));
+        def b2 = p.scheduleBuild2(0).waitForStart()
+        assertEquals 2,b2.number
+
+        // now reload
+        p.updateByXml(new StreamSource(p.configFile.file))
+
+        // we should still see the same object for #2 because that's in progress
+        assertSame p.getBuildByNumber(b2.number),b2
+        // build #1 should be reloaded
+        assertNotSame b1,p.getBuildByNumber(1)
+
+        // and reference gets fixed up
+        b1 = p.getBuildByNumber(1)
+        assertSame b1.nextBuild,b2
+        assertSame b2.previousBuild,b1
+    }
+}