[FIXED JENKINS-19623] Avoid getAllItems during getCategoryProjects as…

… it checks permissions and so can be expensive, whereas QueueTaskDispatcher.canTake must be fast.
jenkinsci · Nov 7, 2013 · 9fc87ab · 9fc87ab
1 parent f9dc146
commit 9fc87ab
Show file tree

Hide file tree

Showing 2 changed files with 93 additions and 11 deletions.
diff --git a/src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java b/src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java
@@ -10,9 +10,17 @@
 import hudson.util.FormValidation;
 import hudson.util.ListBoxModel;
 import hudson.Util;
+import hudson.model.Item;
+import hudson.model.listeners.ItemListener;
 
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import jenkins.model.Jenkins;
 
 import net.sf.json.JSONObject;
@@ -80,6 +88,24 @@ public Object readResolve() {
 
         return this;
     }
+
+    @Override protected void setOwner(AbstractProject<?,?> owner) {
+        super.setOwner(owner);
+        if (throttleEnabled && categories != null) {
+            String fullName = owner.getFullName();
+            Map<String,Set<String>> projectsByCategory = ((DescriptorImpl) getDescriptor()).projectsByCategory;
+            synchronized (projectsByCategory) {
+                for (String c : categories) {
+                    Set<String> projects = projectsByCategory.get(c);
+                    if (projects == null) {
+                        projects = new HashSet<String>();
+                        projectsByCategory.put(c, projects);
+                    }
+                    projects.add(fullName);
+                }
+            }
+        }
+    }
 
     public boolean getThrottleEnabled() {
         return throttleEnabled;
@@ -110,21 +136,50 @@ public Integer getMaxConcurrentTotal() {
     static List<AbstractProject<?,?>> getCategoryProjects(String category) {
         assert category != null && !category.equals("");
         List<AbstractProject<?,?>> categoryProjects = new ArrayList<AbstractProject<?, ?>>();
-        for (AbstractProject<?,?> p : Jenkins.getInstance().getAllItems(AbstractProject.class)) {
-            ThrottleJobProperty t = p.getProperty(ThrottleJobProperty.class);
-            if (t != null && t.getThrottleEnabled()) {
-                if (t.getCategories() != null && t.getCategories().contains(category)) {
-                    categoryProjects.add(p);
+        Collection<String> projects;
+        Map<String,Set<String>> projectsByCategory = Jenkins.getInstance().getDescriptorByType(DescriptorImpl.class).projectsByCategory;
+        synchronized (projectsByCategory) {
+            Set<String> _projects = projectsByCategory.get(category);
+            projects = _projects != null ? new ArrayList<String>(_projects) : Collections.<String>emptySet();
+        }
+        for (String project : projects) {
+            AbstractProject<?,?> p = Jenkins.getInstance().getItemByFullName(project, AbstractProject.class);
+            // double-check that it still exists and has the right property:
+            if (p != null) {
+                ThrottleJobProperty t = p.getProperty(ThrottleJobProperty.class);
+                if (t != null && t.getThrottleEnabled()) {
+                    if (t.getCategories() != null && t.getCategories().contains(category)) {
+                        categoryProjects.add(p);
+                    }
                 }
             }
         }
         return categoryProjects;
     }
+    @Extension public static final class ItemListenerImpl extends ItemListener {
+        @Override public void onRenamed(Item item, String oldName, String newName) {
+            String parentName = item.getParent().getFullName();
+            String oldFullName = parentName.equals("") ? oldName : parentName + '/' + oldName;
+            String newFullName = item.getFullName();
+            Map<String,Set<String>> projectsByCategory = Jenkins.getInstance().getDescriptorByType(DescriptorImpl.class).projectsByCategory;
+            synchronized (projectsByCategory) {
+                for (Set<String> projects : projectsByCategory.values()) {
+                    if (projects.contains(oldFullName)) { // do not bother removing, we already double-check existence
+                        projects.add(newFullName);
+                    }
+                }
+            }
+        }
+        // do not bother with onDeleted for the same reason
+    }
 
     @Extension
     public static final class DescriptorImpl extends JobPropertyDescriptor {
         private List<ThrottleCategory> categories;
 
+        /** Map from category names, to {@link AbstractProject#getFullName}s of projects currently thought to have an enabled property including that category. */
+        private Map<String,Set<String>> projectsByCategory = new HashMap<String,Set<String>>();
+
         public DescriptorImpl() {
             super(ThrottleJobProperty.class);
             load();

diff --git a/src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java b/src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java
@@ -2,7 +2,11 @@
 
 import hudson.model.AbstractProject;
 import hudson.model.FreeStyleProject;
+import hudson.model.Job;
+import hudson.security.ACL;
+import hudson.security.AuthorizationStrategy;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import org.jvnet.hudson.test.Bug;
@@ -23,14 +27,37 @@ public void testGetCategoryProjects() throws Exception {
         FreeStyleProject p4 = createFreeStyleProject("p4");
         p4.addProperty(new ThrottleJobProperty(1, 1, Arrays.asList(beta, gamma), true, THROTTLE_OPTION_CATEGORY));
         // TODO when core dep ≥1.480.3, add cloudbees-folder as a test dependency so we can check jobs inside folders
-        assertEquals(Collections.singleton(p3), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(alpha)));
-        assertEquals(new HashSet<AbstractProject<?,?>>(Arrays.asList(p3, p4)), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(beta)));
-        assertEquals(Collections.singleton(p4), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(gamma)));
-        assertEquals(Collections.emptySet(), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects("delta")));
+        assertProjects(alpha, p3);
+        assertProjects(beta, p3, p4);
+        assertProjects(gamma, p4);
+        assertProjects("delta");
         p4.renameTo("p-4");
-        assertEquals(Collections.singleton(p4), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(gamma)));
+        assertProjects(gamma, p4);
         p4.delete();
-        assertEquals(Collections.emptySet(), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(gamma)));
+        assertProjects(gamma);
+        AbstractProject<?,?> p3b = jenkins.<AbstractProject<?,?>>copy(p3, "p3b");
+        assertProjects(beta, p3, p3b);
+    }
+    private void assertProjects(String category, AbstractProject<?,?>... projects) {
+        jenkins.setAuthorizationStrategy(new RejectP1P2AuthorizationStrategy());
+        try {
+            assertEquals(new HashSet<AbstractProject<?,?>>(Arrays.asList(projects)), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(category)));
+        } finally {
+            jenkins.setAuthorizationStrategy(AuthorizationStrategy.UNSECURED); // do not check during e.g. rebuildDependencyGraph from delete
+        }
+    }
+    private static class RejectP1P2AuthorizationStrategy extends AuthorizationStrategy {
+        RejectP1P2AuthorizationStrategy() {}
+        @Override public ACL getRootACL() {
+            return new AuthorizationStrategy.Unsecured().getRootACL();
+        }
+        @Override public Collection<String> getGroups() {
+            return Collections.emptySet();
+        }
+        @Override public ACL getACL(Job<?,?> project) {
+            assertFalse("not even supposed to be looking at " + project, project.getFullName().matches("p1|p2"));
+            return super.getACL(project);
+        }
     }