Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-40771] FlowExecutionList.register (and .unregister) wa…
…s incorrectly loading from disk, causing a race condition with asynchronous saves.
  • Loading branch information
jglick committed Feb 14, 2017
1 parent d061745 commit 643dc71
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 11 deletions.
2 changes: 1 addition & 1 deletion pom.xml
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>2.21</version>
<version>2.22</version>
<relativePath/>
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
Expand Up @@ -41,7 +41,7 @@
*/
@Extension
public class FlowExecutionList implements Iterable<FlowExecution> {
private CopyOnWriteList<FlowExecutionOwner> runningTasks = new CopyOnWriteList<FlowExecutionOwner>();
private final CopyOnWriteList<FlowExecutionOwner> runningTasks = new CopyOnWriteList<>();
private final SingleLaneExecutorService executor = new SingleLaneExecutorService(Timer.get());
private XmlFile configFile;

Expand Down Expand Up @@ -97,6 +97,7 @@ private synchronized void load() {
if (cf.exists()) {
try {
runningTasks.replaceBy((List<FlowExecutionOwner>) cf.read());
LOGGER.log(FINE, "loaded: {0}", runningTasks);
} catch (IOException x) {
LOGGER.log(WARNING, null, x);
}
Expand All @@ -109,21 +110,26 @@ private synchronized void load() {
* are no longer running.
*/
public synchronized void register(final FlowExecutionOwner self) {
load();
if (!runningTasks.contains(self))
if (runningTasks.contains(self)) {
LOGGER.log(WARNING, "{0} was already in the list: {1}", new Object[] {self, runningTasks.getView()});
} else {
runningTasks.add(self);
saveLater();
saveLater();
}
}

public synchronized void unregister(final FlowExecutionOwner self) {
load();
runningTasks.remove(self);
LOGGER.log(FINE, "unregistered {0} so is now {1}", new Object[] {self, runningTasks.getView()});
saveLater();
if (runningTasks.remove(self)) {
LOGGER.log(FINE, "unregistered {0}", new Object[] {self});
saveLater();
} else {
LOGGER.log(WARNING, "{0} was not in the list to begin with: {1}", new Object[] {self, runningTasks.getView()});
}
}

private synchronized void saveLater() {
final List<FlowExecutionOwner> copy = new ArrayList<FlowExecutionOwner>(runningTasks.getView());
final List<FlowExecutionOwner> copy = new ArrayList<>(runningTasks.getView());
LOGGER.log(FINE, "scheduling save of {0}", copy);
try {
executor.submit(new Runnable() {
@Override public void run() {
Expand Down Expand Up @@ -203,7 +209,7 @@ public static class StepExecutionIteratorImpl extends StepExecutionIterator {

@Override
public ListenableFuture<?> apply(final Function<StepExecution, Void> f) {
List<ListenableFuture<?>> all = new ArrayList<ListenableFuture<?>>();
List<ListenableFuture<?>> all = new ArrayList<>();

for (FlowExecution e : list) {
ListenableFuture<List<StepExecution>> execs = e.getCurrentExecutions(false);
Expand Down
@@ -0,0 +1,86 @@
/*
* The MIT License
*
* Copyright 2017 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package org.jenkinsci.plugins.workflow.flow;

import hudson.model.ParametersAction;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.StringParameterDefinition;
import hudson.model.StringParameterValue;
import hudson.model.queue.QueueTaskFuture;
import java.util.logging.Level;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.junit.ClassRule;
import org.junit.Test;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.RestartableJenkinsRule;

public class FlowExecutionListTest {

@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
@Rule public RestartableJenkinsRule rr = new RestartableJenkinsRule();
@Rule public LoggerRule logging = new LoggerRule().record(FlowExecutionList.class, Level.FINE);

@Issue("JENKINS-40771")
@Test public void simultaneousRegister() throws Exception {
rr.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
WorkflowJob p = rr.j.createProject(WorkflowJob.class, "p");
{ // make sure there is an initial FlowExecutionList.xml
p.setDefinition(new CpsFlowDefinition("", true));
rr.j.buildAndAssertSuccess(p);
}
p.setDefinition(new CpsFlowDefinition("echo params.key; sleep 5", true));
p.addProperty(new ParametersDefinitionProperty(new StringParameterDefinition("key", null)));
QueueTaskFuture<WorkflowRun> f1 = p.scheduleBuild2(0, new ParametersAction(new StringParameterValue("key", "one")));
QueueTaskFuture<WorkflowRun> f2 = p.scheduleBuild2(0, new ParametersAction(new StringParameterValue("key", "two")));
f1.waitForStart();
f2.waitForStart();
WorkflowRun b2 = p.getBuildByNumber(2);
assertNotNull(b2);
WorkflowRun b3 = p.getBuildByNumber(3);
assertNotNull(b3);
rr.j.waitForMessage("Sleeping for ", b2);
rr.j.waitForMessage("Sleeping for ", b3);
}
});
rr.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
WorkflowJob p = rr.j.jenkins.getItemByFullName("p", WorkflowJob.class);
WorkflowRun b2 = p.getBuildByNumber(2);
WorkflowRun b3 = p.getBuildByNumber(3);
rr.j.assertBuildStatusSuccess(rr.j.waitForCompletion(b2));
rr.j.assertBuildStatusSuccess(rr.j.waitForCompletion(b3));
}
});
}

}

0 comments on commit 643dc71

Please sign in to comment.