Merge branch 'JENKINS-24763' of github.com:recampbell/jenkins

jenkinsci · Oct 10, 2014 · ef6c042 · ef6c042
2 parents 3addbab + 8bece68
commit ef6c042
Show file tree

Hide file tree

Showing 2 changed files with 119 additions and 30 deletions.
diff --git a/core/src/main/java/hudson/diagnosis/OldDataMonitor.java b/core/src/main/java/hudson/diagnosis/OldDataMonitor.java
@@ -23,6 +23,7 @@
  */
 package hudson.diagnosis;
 
+import com.google.common.base.Predicate;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import hudson.Extension;
 import hudson.XmlFile;
@@ -38,9 +39,11 @@
 import hudson.util.RobustReflectionConverter;
 import hudson.util.VersionNumber;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
 import java.util.logging.Level;
@@ -65,7 +68,6 @@ public class OldDataMonitor extends AdministrativeMonitor {
     private static final Logger LOGGER = Logger.getLogger(OldDataMonitor.class.getName());
 
     private HashMap<SaveableReference,VersionRange> data = new HashMap<SaveableReference,VersionRange>();
-    private boolean updating = false;
 
     static OldDataMonitor get(Jenkins j) {
         return (OldDataMonitor) j.getAdministrativeMonitor("OldData");
@@ -102,7 +104,6 @@ public Map<Saveable,VersionRange> getData() {
     private static void remove(Saveable obj, boolean isDelete) {
         OldDataMonitor odm = get(Jenkins.getInstance());
         synchronized (odm) {
-            if (odm.updating) return; // Skip during doUpgrade or doDiscard
             odm.data.remove(referTo(obj));
             if (isDelete && obj instanceof Job<?,?>)
                 for (Run r : ((Job<?,?>)obj).getBuilds())
@@ -274,19 +275,18 @@ 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) {
-        String thruVerParam = req.getParameter("thruVer");
-        VersionNumber thruVer = thruVerParam.equals("all") ? null : new VersionNumber(thruVerParam);
-        updating = true;
-        for (Iterator<Map.Entry<SaveableReference,VersionRange>> it = data.entrySet().iterator(); it.hasNext();) {
-            Map.Entry<SaveableReference,VersionRange> entry = it.next();
-            VersionNumber version = entry.getValue().max;
-            if (version != null && (thruVer == null || !version.isNewerThan(thruVer))) {
-                it.remove();
-                tryToSave(entry.getKey());
+    public HttpResponse doUpgrade(StaplerRequest req, StaplerResponse rsp) {
+        final String thruVerParam = req.getParameter("thruVer");
+        final VersionNumber thruVer = thruVerParam.equals("all") ? null : new VersionNumber(thruVerParam);
+
+        saveAndRemoveEntries( new Predicate<Map.Entry<SaveableReference,VersionRange>>() {
+            @Override
+            public boolean apply(Map.Entry<SaveableReference, VersionRange> entry) {
+                VersionNumber version = entry.getValue().max;
+                return version != null && (thruVer == null || !version.isNewerThan(thruVer));
             }
-        }
-        updating = false;
+        });
+
         return HttpResponses.forwardToPreviousPage();
     }
 
@@ -295,28 +295,52 @@ public synchronized HttpResponse doUpgrade(StaplerRequest req, StaplerResponse r
      * Remove those items from the data map.
      */
     @RequirePOST
-    public synchronized HttpResponse doDiscard(StaplerRequest req, StaplerResponse rsp) {
-        updating = true;
-        for (Iterator<Map.Entry<SaveableReference,VersionRange>> it = data.entrySet().iterator(); it.hasNext();) {
-            Map.Entry<SaveableReference,VersionRange> entry = it.next();
-            if (entry.getValue().max == null) {
-                it.remove();
-                tryToSave(entry.getKey());
+    public HttpResponse doDiscard(StaplerRequest req, StaplerResponse rsp) {
+        saveAndRemoveEntries( new Predicate<Map.Entry<SaveableReference,VersionRange>>() {
+            @Override
+            public boolean apply(Map.Entry<SaveableReference, VersionRange> entry) {
+                return entry.getValue().max == null;
             }
-        }
-        updating = false;
+        });
+
         return HttpResponses.forwardToPreviousPage();
     }
 
-    private void tryToSave(SaveableReference ref) {
-        Saveable s = ref.get();
-        if (s != null) {
-            try {
-                s.save();
-            } catch (Exception x) {
-                LOGGER.log(Level.WARNING, "failed to save " + s, x);
+    private void saveAndRemoveEntries(Predicate<Map.Entry<SaveableReference, VersionRange>> matchingPredicate) {
+        /*
+         * Note that there a race condition here: we acquire the lock and get localCopy which includes some
+         * project (say); then we go through our loop and save that project; then someone POSTs a new
+         * config.xml for the project with some old data, causing remove to be called and the project to be
+         * added to data (in the new version); then we hit the end of this method and the project is removed
+         * from data again, even though it again has old data.
+         *
+         * In practice this condition is extremely unlikely, and not a major problem even if it
+         * does occur: just means the user will be prompted to discard less than they should have been (and
+         * would see the warning again after next restart).
+         */
+        Map<SaveableReference,VersionRange> localCopy = null;
+        synchronized (this) {
+            localCopy = new HashMap<SaveableReference,VersionRange>(data);
+        }
+
+        List<SaveableReference> removed = new ArrayList<SaveableReference>();
+        for (Map.Entry<SaveableReference,VersionRange> entry : localCopy.entrySet()) {
+            if (matchingPredicate.apply(entry)) {
+                Saveable s = entry.getKey().get();
+                if (s != null) {
+                    try {
+                        s.save();
+                    } catch (Exception x) {
+                        LOGGER.log(Level.WARNING, "failed to save " + s, x);
+                    }
+                }
+                removed.add(entry.getKey());
             }
         }
+
+        synchronized (this) {
+            data.keySet().removeAll(removed);
+        }
     }
 
     public HttpResponse doIndex(StaplerResponse rsp) throws IOException {

diff --git a/test/src/test/java/hudson/diagnosis/OldDataMonitorTest.java b/test/src/test/java/hudson/diagnosis/OldDataMonitorTest.java
@@ -24,20 +24,37 @@
 
 package hudson.diagnosis;
 
+import hudson.XmlFile;
+import hudson.model.Executor;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.InvisibleAction;
+
+import java.io.File;
+import java.io.IOException;
 import java.lang.ref.WeakReference;
 import java.util.Collections;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import hudson.model.Saveable;
+import hudson.model.listeners.SaveableListener;
+import jenkins.model.Jenkins;
 import jenkins.model.lazy.BuildReference;
 import static org.junit.Assert.*;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MemoryAssert;
 import org.jvnet.hudson.test.recipes.LocalData;
+import org.kohsuke.stapler.Stapler;
 
 public class OldDataMonitorTest {
 
@@ -84,6 +101,54 @@ public class OldDataMonitorTest {
         MemoryAssert.assertGC(ref);
     }
 
+    /**
+     * Note that this doesn't actually run slowly, it just ensures that
+     * the {@link OldDataMonitor#changeListener's onChange()} can complete
+     * while {@link OldDataMonitor#doDiscard(org.kohsuke.stapler.StaplerRequest, org.kohsuke.stapler.StaplerResponse)}
+     * is still running.
+     *
+     */
+    // Test timeout indicates JENKINS-24763 exists
+    @Issue("JENKINS-24763")
+    @Test public void slowDiscard() throws InterruptedException, IOException, ExecutionException {
+        final OldDataMonitor oldDataMonitor = OldDataMonitor.get(r.jenkins);
+        final CountDownLatch ensureEntry= new CountDownLatch(1);
+        final CountDownLatch preventExit = new CountDownLatch(1);
+        Saveable slowSavable = new Saveable() {
+            @Override
+            public void save() throws IOException {
+                try {
+                    ensureEntry.countDown();
+                    preventExit.await();
+                } catch (InterruptedException e) {
+                }
+            }
+        };
+
+        OldDataMonitor.report(slowSavable,(String)null);
+        ExecutorService executors = Executors.newSingleThreadExecutor();
+
+        Future<Void> discardFuture = executors.submit(new Callable<Void>() {
+            @Override
+            public Void call() throws Exception {
+                oldDataMonitor.doDiscard(Stapler.getCurrentRequest(), Stapler.getCurrentResponse());
+                return null;
+            }
+        });
+
+        ensureEntry.await();
+        // test will hang here due to JENKINS-24763
+        File xml = File.createTempFile("OldDataMontiorTest.slowDiscard", "xml");
+        xml.deleteOnExit();
+        OldDataMonitor.changeListener
+                .onChange(new Saveable() {public void save() throws IOException {}},
+                        new XmlFile(xml));
+
+        preventExit.countDown();
+        discardFuture.get();
+
+    }
+
     public static final class BadAction extends InvisibleAction {
         private Object writeReplace() {
             throw new IllegalStateException("broken");