[JENKINS-19544] Whether or not we manage to save an object with old d…

…ata, be sure to remove it from the list, and continue with other objects. Otherwise a bug like JENKINS-20950 can prevent *anything* from being saved, and holds references to all the old data.
jenkinsci · Mar 21, 2014 · 8508fc3 · 8508fc3
1 parent 10eca37
commit 8508fc3
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 7 deletions.
diff --git a/core/src/main/java/hudson/diagnosis/OldDataMonitor.java b/core/src/main/java/hudson/diagnosis/OldDataMonitor.java
@@ -68,6 +68,10 @@ public class OldDataMonitor extends AdministrativeMonitor {
     private HashMap<Saveable,VersionRange> data = new HashMap<Saveable,VersionRange>();
     private boolean updating = false;
 
+    static OldDataMonitor get(Jenkins j) {
+        return (OldDataMonitor) j.getAdministrativeMonitor("OldData");
+    }
+
     public OldDataMonitor() {
         super("OldData");
     }
@@ -86,7 +90,7 @@ public synchronized Map<Saveable,VersionRange> getData() {
     }
 
     private static void remove(Saveable obj, boolean isDelete) {
-        OldDataMonitor odm = (OldDataMonitor) Jenkins.getInstance().getAdministrativeMonitor("OldData");
+        OldDataMonitor odm = get(Jenkins.getInstance());
         synchronized (odm) {
             if (odm.updating) return; // Skip during doUpgrade or doDiscard
             odm.data.remove(obj);
@@ -130,7 +134,7 @@ public void onDeleted(Run run) {
      * @param version Hudson release when the data structure changed.
      */
     public static void report(Saveable obj, String version) {
-        OldDataMonitor odm = (OldDataMonitor) Jenkins.getInstance().getAdministrativeMonitor("OldData");
+        OldDataMonitor odm = get(Jenkins.getInstance());
         synchronized (odm) {
             try {
                 VersionRange vr = odm.data.get(obj);
@@ -184,7 +188,7 @@ public static void report(Saveable obj, Collection<Throwable> errors) {
             }
             return;
         }
-        OldDataMonitor odm = (OldDataMonitor) j.getAdministrativeMonitor("OldData");
+        OldDataMonitor odm = get(j);
         synchronized (odm) {
             VersionRange vr = odm.data.get(obj);
             if (vr != null) vr.extra = buf.toString();
@@ -258,16 +262,20 @@ public HttpResponse doAct(StaplerRequest req, StaplerResponse rsp) throws IOExce
      * Remove those items from the data map.
      */
     @RequirePOST
-    public synchronized HttpResponse doUpgrade(StaplerRequest req, StaplerResponse rsp) throws IOException {
+    public synchronized HttpResponse doUpgrade(StaplerRequest req, StaplerResponse rsp) {
         String thruVerParam = req.getParameter("thruVer");
         VersionNumber thruVer = thruVerParam.equals("all") ? null : new VersionNumber(thruVerParam);
         updating = true;
         for (Iterator<Map.Entry<Saveable,VersionRange>> it = data.entrySet().iterator(); it.hasNext();) {
             Map.Entry<Saveable,VersionRange> entry = it.next();
             VersionNumber version = entry.getValue().max;
             if (version != null && (thruVer == null || !version.isNewerThan(thruVer))) {
-                entry.getKey().save();
                 it.remove();
+                try {
+                    entry.getKey().save();
+                } catch (Exception x) {
+                    LOGGER.log(Level.WARNING, "failed to save " + entry.getKey(), x);
+                }
             }
         }
         updating = false;
@@ -279,13 +287,17 @@ public synchronized HttpResponse doUpgrade(StaplerRequest req, StaplerResponse r
      * Remove those items from the data map.
      */
     @RequirePOST
-    public synchronized HttpResponse doDiscard(StaplerRequest req, StaplerResponse rsp) throws IOException {
+    public synchronized HttpResponse doDiscard(StaplerRequest req, StaplerResponse rsp) {
         updating = true;
         for (Iterator<Map.Entry<Saveable,VersionRange>> it = data.entrySet().iterator(); it.hasNext();) {
             Map.Entry<Saveable,VersionRange> entry = it.next();
             if (entry.getValue().max == null) {
-                entry.getKey().save();
                 it.remove();
+                try {
+                    entry.getKey().save();
+                } catch (Exception x) {
+                    LOGGER.log(Level.WARNING, "failed to save " + entry.getKey(), x);
+                }
             }
         }
         updating = false;

diff --git a/test/src/test/java/hudson/diagnosis/OldDataMonitorTest.java b/test/src/test/java/hudson/diagnosis/OldDataMonitorTest.java
@@ -0,0 +1,67 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2014 Jesse Glick.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.diagnosis;
+
+import hudson.model.FreeStyleProject;
+import hudson.model.InvisibleAction;
+import java.util.Collections;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.recipes.LocalData;
+
+public class OldDataMonitorTest {
+
+    @Rule public JenkinsRule r = new JenkinsRule();
+
+    @Bug(19544)
+    @LocalData
+    @Test public void robustness() throws Exception {
+        OldDataMonitor odm = OldDataMonitor.get(r.jenkins);
+        FreeStyleProject p = r.jenkins.getItemByFullName("busted", FreeStyleProject.class);
+        assertNotNull(p);
+        /*
+        System.err.println(p.getActions());
+        for (Map.Entry<Saveable,OldDataMonitor.VersionRange> entry : odm.getData().entrySet()) {
+            System.err.println(entry.getKey());
+            System.err.println(entry.getValue());
+            System.err.println(entry.getValue().extra);
+        }
+        */
+        assertEquals(Collections.singleton(p), odm.getData().keySet());
+        odm.doDiscard(null, null);
+        assertEquals(Collections.emptySet(), odm.getData().keySet());
+        // did not manage to save p, but at least we are not holding onto a reference to it anymore
+    }
+
+    public static final class BadAction extends InvisibleAction {
+        private Object writeReplace() {
+            throw new IllegalStateException("broken");
+        }
+    }
+
+}
diff --git a/test/src/test/resources/hudson/diagnosis/OldDataMonitorTest/robustness.zip b/test/src/test/resources/hudson/diagnosis/OldDataMonitorTest/robustness.zip