Merge pull request #24 from sflyphotobooks/make_categories_concurrent_modification_safe

Switch Lists used to store categories to a concurrent-safe implementatio...

Author: oleg-nenashev

src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java

@@ -23,6 +23,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.WeakHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import jenkins.model.Jenkins;
@@ -61,7 +62,9 @@ public ThrottleJobProperty(Integer maxConcurrentPerNode,
                                ) {
         this.maxConcurrentPerNode = maxConcurrentPerNode == null ? 0 : maxConcurrentPerNode;
         this.maxConcurrentTotal = maxConcurrentTotal == null ? 0 : maxConcurrentTotal;
-        this.categories = categories;
+        this.categories = categories == null ?
+                new CopyOnWriteArrayList<String>() :
+                new CopyOnWriteArrayList<String>(categories);
         this.throttleEnabled = throttleEnabled;
         this.throttleOption = throttleOption;
         this.matrixOptions = matrixOptions;
@@ -76,7 +79,7 @@ public Object readResolve() {
             configVersion = 0L;
         }
         if (categories == null) {
-            categories = new ArrayList<String>();
+            categories = new CopyOnWriteArrayList<String>();
         }
         if (category != null) {
             categories.add(category);
@@ -212,7 +215,7 @@ public static final class DescriptorImpl extends JobPropertyDescriptor {
                  = new HashMap<String,Map<ThrottleJobProperty,Void>>();
         /** A sync object for {@link #propertiesByCategory} */
         private final transient Object propertiesByCategoryLock = new Object();
-         
+
         public DescriptorImpl() {
             super(ThrottleJobProperty.class);
             synchronized(propertiesByCategoryLock) {
@@ -227,7 +230,7 @@ public DescriptorImpl() {
                 }
             }
         }
-     
+
         @Override
         public String getDisplayName() {
             return "Throttle Concurrent Builds";
@@ -290,12 +293,12 @@ public ThrottleCategory getCategoryByName(String categoryName) {
         }
 
         public void setCategories(List<ThrottleCategory> categories) {
-            this.categories = categories;
+            this.categories = new CopyOnWriteArrayList<ThrottleCategory>(categories);
         }
 
         public List<ThrottleCategory> getCategories() {
             if (categories == null) {
-                categories = new ArrayList<ThrottleCategory>();
+                categories = new CopyOnWriteArrayList<ThrottleCategory>();
             }
 
             return categories;

src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java

@@ -5,16 +5,16 @@
 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;
 import org.jvnet.hudson.test.HudsonTestCase;
 
+import java.util.*;
+import java.util.concurrent.CopyOnWriteArrayList;
+
 public class ThrottleJobPropertyTest extends HudsonTestCase {
 
     private static final String THROTTLE_OPTION_CATEGORY = "category"; // TODO move this into ThrottleJobProperty and use consistently; same for "project"
+    private final Random random = new Random(System.currentTimeMillis());
 
     @Bug(19623)
     public void testGetCategoryProjects() throws Exception {
@@ -40,6 +40,88 @@ public void testGetCategoryProjects() throws Exception {
         p3.removeProperty(ThrottleJobProperty.class);
         assertProjects(beta, p3b);
     }
+
+
+
+    public void testToString_withNulls(){
+        ThrottleJobProperty tjp = new ThrottleJobProperty(0,0, null, false, null, ThrottleMatrixProjectOptions.DEFAULT);
+        assertNotNull(tjp.toString());
+    }
+
+    public void testThrottleJob_constructor_should_store_arguments() {
+        Integer expectedMaxConcurrentPerNode = anyInt();
+        Integer expectedMaxConcurrentTotal = anyInt();
+        List<String> expectedCategories = Collections.emptyList();
+        boolean expectedThrottleEnabled = anyBoolean();
+        String expectedThrottleOption = anyString();
+
+        ThrottleJobProperty property = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                ThrottleMatrixProjectOptions.DEFAULT);
+
+        assertEquals(expectedMaxConcurrentPerNode, property.getMaxConcurrentPerNode());
+        assertEquals(expectedMaxConcurrentTotal, property.getMaxConcurrentTotal());
+        assertEquals(expectedCategories, property.getCategories());
+        assertEquals(expectedThrottleEnabled, property.getThrottleEnabled());
+        assertEquals(expectedThrottleOption, property.getThrottleOption());
+    }
+
+    public void testThrottleJob_should_copy_categories_to_concurrency_safe_list() {
+        final String category = anyString();
+
+        ArrayList<String> unsafeList = new ArrayList<String>() {{
+            add(category);
+        }};
+
+        ThrottleJobProperty property = new ThrottleJobProperty(anyInt(),
+                anyInt(),
+                unsafeList,
+                anyBoolean(),
+                "throttle_option",
+                ThrottleMatrixProjectOptions.DEFAULT);
+
+        List<String> storedCategories = property.getCategories();
+        assertEquals("contents of original and stored list should be the equal", unsafeList, storedCategories);
+        assertTrue("expected unsafe list to be converted to a converted to some other concurrency-safe impl",
+                unsafeList != storedCategories);
+        assertTrue(storedCategories instanceof CopyOnWriteArrayList);
+    }
+
+    public void testThrottleJob_constructor_handles_null_categories(){
+        ThrottleJobProperty property = new ThrottleJobProperty(anyInt(),
+                anyInt(),
+                null,
+                anyBoolean(),
+                "throttle_option",
+                ThrottleMatrixProjectOptions.DEFAULT);
+
+        assertEquals(Collections.<String>emptyList(), property.getCategories());
+    }
+
+    public void testDescriptorImpl_should_a_concurrency_safe_list_for_categories(){
+        ThrottleJobProperty.DescriptorImpl descriptor = new ThrottleJobProperty.DescriptorImpl();
+
+        assertTrue(descriptor.getCategories() instanceof CopyOnWriteArrayList);
+
+        final ThrottleJobProperty.ThrottleCategory category = new ThrottleJobProperty.ThrottleCategory(
+                anyString(), anyInt(), anyInt(), null);
+
+        ArrayList<ThrottleJobProperty.ThrottleCategory> unsafeList =
+                new ArrayList<ThrottleJobProperty.ThrottleCategory>() {{
+                    add(category);
+                }};
+
+
+        descriptor.setCategories(unsafeList);
+        List<ThrottleJobProperty.ThrottleCategory> storedCategories = descriptor.getCategories();
+        assertEquals("contents of original and stored list should be the equal", unsafeList, storedCategories);
+        assertTrue("expected unsafe list to be converted to a converted to some other concurrency-safe impl",
+                unsafeList != storedCategories);
+        assertTrue(storedCategories instanceof CopyOnWriteArrayList);
+    }
+
+
     private void assertProjects(String category, AbstractProject<?,?>... projects) {
         jenkins.setAuthorizationStrategy(new RejectAllAuthorizationStrategy());
         try {
@@ -61,4 +143,17 @@ private static class RejectAllAuthorizationStrategy extends AuthorizationStrateg
             return super.getACL(project);
         }
     }
+
+    private String anyString() {
+        return "concurrency_" + anyInt();
+    }
+
+    private boolean anyBoolean() {
+        return random.nextBoolean();
+    }
+
+    private int anyInt() {
+        return random.nextInt(10000);
+    }
+
 }