Merge pull request #40 from jenkinsci/JENKINS-48465-fix-concurrent-mo…

…dification-exception [JENKINS-48465] Fix ConcurrentModificationException in PlotBuildAction
jenkinsci · Jan 3, 2018 · 56fc218 · 56fc218
2 parents 490c98c + 0f03011
commit 56fc218
Show file tree

Hide file tree

Showing 2 changed files with 121 additions and 5 deletions.
diff --git a/src/main/java/hudson/plugins/plot/PlotBuildAction.java b/src/main/java/hudson/plugins/plot/PlotBuildAction.java
@@ -3,10 +3,10 @@
 import hudson.model.Action;
 import hudson.model.InvisibleAction;
 import hudson.model.Run;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 import jenkins.tasks.SimpleBuildStep;
 import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.export.ExportedBean;
@@ -20,11 +20,11 @@ public class PlotBuildAction extends InvisibleAction
 
     PlotBuildAction(Run<?, ?> run, List<Plot> plots) {
         this.run = run;
-        this.plots = plots;
+        this.plots = new CopyOnWriteArrayList<>(plots);
     }
 
     @Override
-    public synchronized Collection<? extends Action> getProjectActions() {
+    public Collection<? extends Action> getProjectActions() {
         return Collections.<Action>singleton(new PlotAction(run.getParent(), plots));
     }
 
@@ -33,9 +33,9 @@ public Object getTarget() {
         return null;
     }
 
-    synchronized void addPlots(List<Plot> plots) {
+    void addPlots(List<Plot> plots) {
         if (this.plots == null) {
-            this.plots = new ArrayList<>();
+            this.plots = new CopyOnWriteArrayList<>();
         }
         this.plots.addAll(plots);
     }

diff --git a/src/test/java/hudson/plugins/plot/PlotBuildActionTest.java b/src/test/java/hudson/plugins/plot/PlotBuildActionTest.java
@@ -0,0 +1,116 @@
+package hudson.plugins.plot;
+
+import com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider;
+import com.thoughtworks.xstream.converters.reflection.ReflectionProvider;
+import hudson.model.Run;
+import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
+import java.util.List;
+import java.util.Random;
+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.FutureTask;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+
+import static org.junit.Assert.fail;
+
+public class PlotBuildActionTest {
+
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+    private PlotBuildAction plotBuildAction;
+
+    @Before
+    public void setUp() throws Exception {
+        final Run<?, ?> run = r.buildAndAssertSuccess(r.createFreeStyleProject());
+        final List<Plot> plots = new ArrayList<>();
+        for (int i = 0; i < 30; i++) {
+            Plot p = new Plot();
+            p.title = String.valueOf(i);
+            plots.add(p);
+        }
+        plotBuildAction = new PlotBuildAction(run, plots);
+    }
+
+    @Issue("JENKINS-48465")
+    @Test
+    public void checksNoConcurrentModificationExceptionIsThrownForPlotsListAccess()
+            throws Exception {
+        int tasksCount = 10;
+        ExecutorService executorService = Executors.newFixedThreadPool(2);
+        List<FutureTask<Object>> tasks = new ArrayList<>();
+        CountDownLatch latch = new CountDownLatch(tasksCount);
+
+        simulateConcurrentModificationException(executorService, tasksCount, tasks, latch);
+
+        waitForAllThreadsToFinish(executorService, latch);
+        assertNoConcurrentModificationExceptionThrown(tasks);
+    }
+
+    private void simulateConcurrentModificationException(ExecutorService executorService,
+            int tasksCount, List<FutureTask<Object>> tasks, final CountDownLatch latch) {
+        for (int i = 0; i < tasksCount; i++) {
+            FutureTask<Object> task = new FutureTask<>(new Callable<Object>() {
+                @Override
+                public Object call() throws Exception {
+                    try {
+                        Thread.sleep(new Random().nextInt(100));
+                        // using PureJavaReflectionProvider just because it's used in Jenkins
+                        // close to "real world"
+                        PureJavaReflectionProvider provider = new PureJavaReflectionProvider();
+                        provider.visitSerializableFields(plotBuildAction,
+                                new ReflectionProvider.Visitor() {
+                                    @Override
+                                    public void visit(String fieldName, Class fieldType,
+                                            Class definedIn, Object value) {
+                                        if (value != null && value instanceof List) {
+                                            List<Plot> plots = (List<Plot>) value;
+                                            // simulate ConcurrentModificationException
+                                            for (Plot p : plots) {
+                                                if (plots.size() > 0) {
+                                                    plots.remove(p);
+                                                }
+                                            }
+                                        }
+                                    }
+                                });
+                    } finally {
+                        latch.countDown();
+                    }
+                    return null;
+                }
+            });
+            tasks.add(task);
+            executorService.submit(task);
+        }
+    }
+
+    private void waitForAllThreadsToFinish(ExecutorService executorService, CountDownLatch latch) {
+        try {
+            latch.await();
+        } catch (InterruptedException e) {
+            e.printStackTrace();
+        }
+        executorService.shutdown();
+    }
+
+    private void assertNoConcurrentModificationExceptionThrown(List<FutureTask<Object>> tasks)
+            throws InterruptedException {
+        try {
+            // we expect here no ConcurrentModificationException
+            // otherwise access to plots list is not synchronized
+            for (FutureTask task : tasks) {
+                task.get();
+            }
+        } catch (ExecutionException | ConcurrentModificationException e) {
+            fail("Access to PlotBuildAction#plots list is not synchronized");
+        }
+    }
+}