Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[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.
  • Loading branch information
kohsuke committed Jul 2, 2013
1 parent f014d7a commit 7facc77
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 40 deletions.
3 changes: 3 additions & 0 deletions changelog.html
Expand Up @@ -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>)
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/matrix/MatrixProject.java
Expand Up @@ -429,7 +429,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;
Expand Down
19 changes: 12 additions & 7 deletions core/src/main/java/hudson/model/AbstractProject.java
Expand Up @@ -130,6 +130,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;

Expand Down Expand Up @@ -239,7 +240,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
Expand Down Expand Up @@ -308,6 +311,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)
Expand All @@ -326,9 +330,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;
}
Expand Down Expand Up @@ -1647,8 +1652,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();
}

/**
Expand Down Expand Up @@ -1975,8 +1980,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))) {
Expand Down
27 changes: 17 additions & 10 deletions core/src/main/java/hudson/model/Project.java
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down
Expand Up @@ -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;
Expand Down
41 changes: 20 additions & 21 deletions test/src/test/groovy/hudson/model/AbstractProjectTest.groovy
Expand Up @@ -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;
Expand All @@ -38,32 +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.ResourceBundle;
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;

/**
Expand Down Expand Up @@ -448,4 +435,16 @@ public class AbstractProjectTest extends HudsonTestCase {
// request should fail
}
}

/**
* We used to store {@link AbstractProject#triggers} as {@link Vector}, so make sure
* we can still read back the configuration from that.
*/
public void testVectorTriggers() {
AbstractProject j = jenkins.createProjectFromXML("foo", getClass().getResourceAsStream("AbstractProjectTest/vectorTriggers.xml"))
assert j.triggers().size()==1
def t = j.triggers()[0]
assert t.class==SCMTrigger.class;
assert t.spec=="*/10 * * * *"
}
}
@@ -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>

2 comments on commit 7facc77

@kutzi
Copy link
Member

@kutzi kutzi commented on 7facc77 Jul 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jglick
Copy link
Member

@jglick jglick commented on 7facc77 Jul 29, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused: JENKINS-18677

Please sign in to comment.