[FIXED JENKINS-18589]

Use DescribableList to handle the copy-on-write semantics correctly. The vector class just doesn't cut it, and we've been setting a new value to this field, which will violates all sorts of the concurrent programming practice. This change has the nice side effect of removing {{class="vector"}} from the persisted XML. A test is added to make sure we can still read back such an XML. (cherry picked from commit 7facc77) Conflicts: test/src/test/groovy/hudson/model/AbstractProjectTest.groovy
jenkinsci · Sep 20, 2013 · 5407d9f · 5407d9f
1 parent 68bde83
commit 5407d9f
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 39 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,6 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    Fixed a dead lock in the <tt>Project</tt> class and improved the signature of the persisted XML form a bit.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-18589">issue 18589</a>)
   <li class=bug>
     Improved memory efficiency in parsing test reports with large stdio output files.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-15382">issue 15382</a>)

diff --git a/core/src/main/java/hudson/matrix/MatrixProject.java b/core/src/main/java/hudson/matrix/MatrixProject.java
@@ -418,7 +418,7 @@ protected List<Action> createTransientActions() {
             r.addAll(step.getProjectActions(this));
         for (BuildWrapper step : buildWrappers)
             r.addAll(step.getProjectActions(this));
-        for (Trigger<?> trigger : triggers)
+        for (Trigger<?> trigger : triggers())
             r.addAll(trigger.getProjectActions());
 
         return r;

diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java
@@ -125,6 +125,7 @@
 import java.util.TreeMap;
 import java.util.Vector;
 import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -234,7 +235,9 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
     /**
      * List of all {@link Trigger}s for this project.
      */
-    protected List<Trigger<?>> triggers = new Vector<Trigger<?>>();
+    protected volatile DescribableList<Trigger<?>,TriggerDescriptor> triggers = new DescribableList<Trigger<?>,TriggerDescriptor>(this);
+    private AtomicReferenceFieldUpdater<AbstractProject,List> triggersUpdater
+            = AtomicReferenceFieldUpdater.newUpdater(AbstractProject.class,List.class,"triggers");
 
     /**
      * {@link Action}s contributed from subsidiary objects associated with
@@ -303,6 +306,7 @@ public void onLoad(ItemGroup<? extends Item> parent, String name) throws IOExcep
             }
         }
         this.builds = builds;
+        triggers().setOwner(this);
         for (Trigger t : triggers())
             t.start(this, Items.updatingByXml.get());
         if(scm==null)
@@ -321,9 +325,10 @@ public R create(File dir) throws IOException {
         });
     }
 
-    private synchronized List<Trigger<?>> triggers() {
+    @WithBridgeMethods(List.class)
+    protected DescribableList<Trigger<?>,TriggerDescriptor> triggers() {
         if (triggers == null) {
-            triggers = new Vector<Trigger<?>>();
+            triggersUpdater.compareAndSet(this,null,new DescribableList<Trigger<?>,TriggerDescriptor>(this));
         }
         return triggers;
     }
@@ -1634,8 +1639,8 @@ void removeFromList(Descriptor<T> item, List<T> collection) throws IOException {
     }
 
     @SuppressWarnings("unchecked")
-    public synchronized Map<TriggerDescriptor,Trigger> getTriggers() {
-        return (Map)Descriptor.toMap(triggers());
+    public Map<TriggerDescriptor,Trigger<?>> getTriggers() {
+        return triggers().toMap();
     }
 
     /**
@@ -1959,8 +1964,8 @@ protected void submit(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
 
         for (Trigger t : triggers())
             t.stop();
-        triggers = buildDescribable(req, Trigger.for_(this));
-        for (Trigger t : triggers)
+        triggers.replaceBy(buildDescribable(req, Trigger.for_(this)));
+        for (Trigger t : triggers())
             t.start(this,true);
 
         for (Publisher _t : Descriptor.newInstancesFromHeteroList(req, json, "publisher", Jenkins.getInstance().getExtensionList(BuildTrigger.DescriptorImpl.class))) {

diff --git a/core/src/main/java/hudson/model/Project.java b/core/src/main/java/hudson/model/Project.java
@@ -47,6 +47,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 
 /**
  * Buildable software project.
@@ -59,17 +60,23 @@ public abstract class Project<P extends Project<P,B>,B extends Build<P,B>>
     /**
      * List of active {@link Builder}s configured for this project.
      */
-    private DescribableList<Builder,Descriptor<Builder>> builders;
+    private volatile DescribableList<Builder,Descriptor<Builder>> builders;
+    private static final AtomicReferenceFieldUpdater<Project,DescribableList> buildersSetter
+            = AtomicReferenceFieldUpdater.newUpdater(Project.class,DescribableList.class,"builders");
 
     /**
      * List of active {@link Publisher}s configured for this project.
      */
-    private DescribableList<Publisher,Descriptor<Publisher>> publishers;
+    private volatile DescribableList<Publisher,Descriptor<Publisher>> publishers;
+    private static final AtomicReferenceFieldUpdater<Project,DescribableList> publishersSetter
+            = AtomicReferenceFieldUpdater.newUpdater(Project.class,DescribableList.class,"publishers");
 
     /**
      * List of active {@link BuildWrapper}s configured for this project.
      */
-    private DescribableList<BuildWrapper,Descriptor<BuildWrapper>> buildWrappers;
+    private volatile DescribableList<BuildWrapper,Descriptor<BuildWrapper>> buildWrappers;
+    private static final AtomicReferenceFieldUpdater<Project,DescribableList> buildWrappersSetter
+            = AtomicReferenceFieldUpdater.newUpdater(Project.class,DescribableList.class,"buildWrappers");
 
     /**
      * Creates a new project.
@@ -103,16 +110,16 @@ public Map<Descriptor<Publisher>,Publisher> getPublishers() {
         return getPublishersList().toMap();
     }
 
-    public synchronized DescribableList<Builder,Descriptor<Builder>> getBuildersList() {
+    public DescribableList<Builder,Descriptor<Builder>> getBuildersList() {
         if (builders == null) {
-            builders = new DescribableList<Builder,Descriptor<Builder>>(this);
+            buildersSetter.compareAndSet(this,null,new DescribableList<Builder,Descriptor<Builder>>(this));
         }
         return builders;
     }
 
-    public synchronized DescribableList<Publisher,Descriptor<Publisher>> getPublishersList() {
+    public DescribableList<Publisher,Descriptor<Publisher>> getPublishersList() {
         if (publishers == null) {
-            publishers = new DescribableList<Publisher,Descriptor<Publisher>>(this);
+            publishersSetter.compareAndSet(this,null,new DescribableList<Publisher,Descriptor<Publisher>>(this));
         }
         return publishers;
     }
@@ -121,9 +128,9 @@ public Map<Descriptor<BuildWrapper>,BuildWrapper> getBuildWrappers() {
         return getBuildWrappersList().toMap();
     }
 
-    public synchronized DescribableList<BuildWrapper, Descriptor<BuildWrapper>> getBuildWrappersList() {
+    public DescribableList<BuildWrapper, Descriptor<BuildWrapper>> getBuildWrappersList() {
         if (buildWrappers == null) {
-            buildWrappers = new DescribableList<BuildWrapper,Descriptor<BuildWrapper>>(this);
+            buildWrappersSetter.compareAndSet(this,null,new DescribableList<BuildWrapper,Descriptor<BuildWrapper>>(this));
         }
         return buildWrappers;
     }
@@ -211,7 +218,7 @@ protected List<Action> createTransientActions() {
             r.addAll(step.getProjectActions(this));
         for (BuildWrapper step : getBuildWrappers().values())
             r.addAll(step.getProjectActions(this));
-        for (Trigger trigger : getTriggers().values())
+        for (Trigger trigger : triggers())
             r.addAll(trigger.getProjectActions());
 
         return r;

diff --git a/maven-plugin/src/main/java/hudson/maven/AbstractMavenProject.java b/maven-plugin/src/main/java/hudson/maven/AbstractMavenProject.java
@@ -184,7 +184,7 @@ protected List<Action> createTransientActions() {
         addTransientActionsFromBuild(getLastBuild(),r,added);
         addTransientActionsFromBuild(getLastSuccessfulBuild(),r,added);
 
-        for (Trigger<?> trigger : triggers)
+        for (Trigger<?> trigger : triggers())
             r.addAll(trigger.getProjectActions());
 
         return r;

diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy
@@ -26,9 +26,7 @@ package hudson.model;
 import com.gargoylesoftware.htmlunit.ElementNotFoundException;
 import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import com.gargoylesoftware.htmlunit.HttpMethod;
-import com.gargoylesoftware.htmlunit.WebRequestSettings;
-import com.gargoylesoftware.htmlunit.html.HtmlForm;
-import com.gargoylesoftware.htmlunit.html.HtmlInput;
+import com.gargoylesoftware.htmlunit.WebRequestSettings
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.security.*;
 import hudson.tasks.BuildTrigger;
@@ -38,31 +36,21 @@ import hudson.Launcher;
 import hudson.FilePath;
 import hudson.Functions;
 import hudson.Util;
-import hudson.tasks.ArtifactArchiver;
+import hudson.tasks.ArtifactArchiver
+import hudson.triggers.SCMTrigger;
 import hudson.util.StreamTaskListener;
-import hudson.util.OneShotEvent;
-import java.io.IOException;
-
+import hudson.util.OneShotEvent
 import jenkins.model.Jenkins;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
-import org.jvnet.hudson.test.HudsonTestCase;
-import org.jvnet.hudson.test.HudsonTestCase.WebClient;
+import org.jvnet.hudson.test.HudsonTestCase
 import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.MemoryAssert;
 import org.jvnet.hudson.test.recipes.PresetData;
-import org.jvnet.hudson.test.recipes.PresetData.DataSet;
-
-import java.io.File;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.Future;
+import org.jvnet.hudson.test.recipes.PresetData.DataSet
 import org.apache.commons.io.FileUtils;
-import java.lang.ref.WeakReference;
-import java.net.HttpURLConnection;
-import java.net.URL;
+import java.lang.ref.WeakReference
+
 import org.jvnet.hudson.test.MockFolder;
 
 /**

diff --git a/test/src/test/resources/hudson/model/AbstractProjectTest/vectorTriggers.xml b/test/src/test/resources/hudson/model/AbstractProjectTest/vectorTriggers.xml
@@ -0,0 +1,8 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <triggers class="vector">
+    <hudson.triggers.SCMTrigger>
+      <spec>*/10 * * * *</spec>
+    </hudson.triggers.SCMTrigger>
+  </triggers>
+</project>