Merge pull request #63 from jglick/hang-JENKINS-34939

[JENKINS-34939] Hold the lock on this folder only while deleting itself, not its children
jenkinsci · May 25, 2016 · 9987689 · 9987689
2 parents 645d3c1 + a88aa7c
commit 9987689
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 2 deletions.
diff --git a/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java b/src/main/java/com/cloudbees/hudson/plugins/folder/AbstractFolder.java
@@ -609,9 +609,13 @@ public void onDeleted(TopLevelItem item) throws IOException {
     }
 
     @Override
-    protected void performDelete() throws IOException, InterruptedException {
+    public void delete() throws IOException, InterruptedException {
+        // Some parts copied from AbstractItem.
+        checkPermission(DELETE);
         // delete individual items first
         // (disregard whether they would be deletable in isolation)
+        // JENKINS-34939: do not hold the monitor on this folder while deleting them
+        // (thus we cannot do this inside performDelete)
         SecurityContext orig = ACL.impersonate(ACL.SYSTEM);
         try {
             for (Item i : new ArrayList<Item>(items.values())) {
@@ -627,7 +631,11 @@ protected void performDelete() throws IOException, InterruptedException {
         } finally {
             SecurityContextHolder.setContext(orig);
         }
-        super.performDelete();
+        synchronized (this) {
+            performDelete();
+        }
+        getParent().onDeleted(AbstractFolder.this);
+        Jenkins.getActiveInstance().rebuildDependencyGraphAsync();
     }
 
     @Override

diff --git a/src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java b/src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java
@@ -28,12 +28,14 @@
 import com.gargoylesoftware.htmlunit.html.HtmlInput;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import com.gargoylesoftware.htmlunit.html.HtmlRadioButtonInput;
+import hudson.model.Actionable;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.Item;
 import hudson.model.Job;
 import hudson.model.ListView;
 import hudson.model.User;
+import hudson.model.listeners.ItemListener;
 import hudson.search.SearchItem;
 import hudson.security.ACL;
 import hudson.security.WhoAmI;
@@ -46,14 +48,22 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.concurrent.Callable;
 import jenkins.model.Jenkins;
+import jenkins.util.Timer;
 import org.acegisecurity.AccessDeniedException;
 import static org.junit.Assert.*;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MockAuthorizationStrategy;
 import org.jvnet.hudson.test.SleepBuilder;
+import org.jvnet.hudson.test.TestExtension;
 import org.jvnet.hudson.test.recipes.LocalData;
 
 public class FolderTest {
@@ -153,6 +163,40 @@ private void copyFromGUI(Folder f, JenkinsRule.WebClient wc, String fromName, St
         assertTrue(n.getItem("child2") instanceof FreeStyleProject);
     }
 
+    @Issue("JENKINS-34939")
+    @Test public void delete() throws Exception {
+        Folder d1 = r.jenkins.createProject(Folder.class, "d1");
+        d1.createProject(FreeStyleProject.class, "p1");
+        d1.createProject(FreeStyleProject.class, "p2");
+        d1.createProject(Folder.class, "d2").createProject(FreeStyleProject.class, "p4");
+        d1.delete();
+        assertEquals("AbstractFolder.items is sorted by name so we can predict deletion order",
+            "{d1=[d1], d1/d2=[d1, d1/d2, d1/p1, d1/p2], d1/d2/p4=[d1, d1/d2, d1/d2/p4, d1/p1, d1/p2], d1/p1=[d1, d1/p1, d1/p2], d1/p2=[d1, d1/p2]}",
+            DeleteListener.whatRemainedWhenDeleted.toString());
+    }
+    @TestExtension("delete") public static class DeleteListener extends ItemListener {
+        static Map<String,Set<String>> whatRemainedWhenDeleted = new TreeMap<String,Set<String>>();
+        @Override public void onDeleted(Item item) {
+            try {
+                // Access metadata from another thread.
+                whatRemainedWhenDeleted.put(item.getFullName(), Timer.get().submit(new Callable<Set<String>>() {
+                    @Override public Set<String> call() throws Exception {
+                        Set<String> remaining = new TreeSet<String>();
+                        for (Item i : Jenkins.getActiveInstance().getAllItems()) {
+                            remaining.add(i.getFullName());
+                            if (i instanceof Actionable) {
+                                ((Actionable) i).getAllActions();
+                            }
+                        }
+                        return remaining;
+                    }
+                }).get());
+            } catch (Exception x) {
+                assert false : x;
+            }
+        }
+    }
+
     /**
      * This is more of a test of the core, but make sure the triggers resolve between ourselves.
      */