Skip to content

Commit 6713aed

Browse files
jglickjglick
authored
Merge pull request jenkinsci#263 from dwnusbaum/robust-action-deserialization
Use lists instead of arrays for `FlowNode` actions to make deserialization more robust
2 parents 5013153 + d682666 commit 6713aed

File tree

26 files changed

+473
-12
lines changed

26 files changed

+473
-12
lines changed

‎src/main/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorage.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import java.lang.reflect.Field;
4242
import java.lang.reflect.InvocationTargetException;
4343
import java.lang.reflect.Method;
44+
import java.util.ArrayList;
4445
import java.util.Arrays;
4546
import java.util.Collections;
4647
import java.util.HashMap;
@@ -138,7 +139,7 @@ public void storeNode(@NonNull FlowNode n, boolean delayWritingActions) throws I
138139
if (t != null) {
139140
t.node = n;
140141
List<Action> act = n.getActions();
141-
t.actions = act.toArray(new Action[act.size()]);
142+
t.actions = new ArrayList<>(act);
142143
} else {
143144
getOrLoadNodes().put(n.getId(), new Tag(n, n.getActions()));
144145
}
@@ -203,7 +204,7 @@ public void saveActions(@NonNull FlowNode node, @NonNull List<Action> actions) t
203204
if (t != null) {
204205
t.node = node;
205206
List<Action> act = node.getActions();
206-
t.actions = act.toArray(new Action[act.size()]);
207+
t.actions = new ArrayList<>(act);
207208
} else {
208209
map.put(node.getId(), new Tag(node, actions));
209210
}
@@ -222,11 +223,11 @@ public boolean isPersistedFully() {
222223
*/
223224
private static class Tag {
224225
/* @NonNull except perhaps after deserialization */ FlowNode node;
225-
private @CheckForNull Action[] actions;
226+
private @CheckForNull List<Action> actions;
226227

227228
private Tag(@NonNull FlowNode node, @NonNull List<Action> actions) {
228229
this.node = node;
229-
this.actions = actions.isEmpty() ? null : actions.toArray(new Action[actions.size()]);
230+
this.actions = actions.isEmpty() ? null : new ArrayList<>(actions);
230231
}
231232

232233
private void storeActions() { // We've already loaded the actions, may as well store them to the FlowNode
@@ -238,7 +239,7 @@ private void storeActions() { // We've already loaded the actions, may as well
238239
}
239240

240241
public @NonNull List<Action> actions() {
241-
return actions != null ? Arrays.asList(actions) : Collections.emptyList();
242+
return actions != null ? Collections.unmodifiableList(actions) : Collections.emptyList();
242243
}
243244
}
244245

‎src/main/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamFlowNodeStorage.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -226,11 +226,11 @@ private Tag load(String id) throws IOException {
226226
*/
227227
private static class Tag {
228228
final /* @NonNull except perhaps after deserialization */ FlowNode node;
229-
private final @CheckForNull Action[] actions;
229+
private final @CheckForNull List<Action> actions;
230230

231231
private Tag(@NonNull FlowNode node, @NonNull List<Action> actions) {
232232
this.node = node;
233-
this.actions = actions.isEmpty() ? null : actions.toArray(new Action[actions.size()]);
233+
this.actions = actions.isEmpty() ? null : new ArrayList<>(actions);
234234
}
235235

236236
private void storeActions() { // We've already loaded the actions, may as well store them to the FlowNode
@@ -242,7 +242,7 @@ private void storeActions() { // We've already loaded the actions, may as well
242242
}
243243

244244
public @NonNull List<Action> actions() {
245-
return actions != null ? Arrays.asList(actions) : Collections.emptyList();
245+
return actions != null ? Collections.unmodifiableList(actions) : Collections.emptyList();
246246
}
247247
}
248248

‎src/test/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest.java‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,24 +24,35 @@
2424

2525
package org.jenkinsci.plugins.workflow.support.storage;
2626

27+
import hudson.model.Result;
28+
import hudson.util.RobustReflectionConverter;
2729
import java.io.File;
2830
import java.util.ArrayList;
31+
import java.util.logging.Level;
2932
import java.util.stream.Collectors;
3033
import java.util.stream.IntStream;
3134
import javax.xml.parsers.DocumentBuilderFactory;
32-
import static org.hamcrest.MatcherAssert.assertThat;
33-
import static org.hamcrest.Matchers.is;
35+
import org.jenkinsci.plugins.workflow.actions.LabelAction;
3436
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
37+
import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
3538
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
3639
import org.junit.Rule;
3740
import org.junit.Test;
3841
import org.jvnet.hudson.test.JenkinsRule;
42+
import org.jvnet.hudson.test.LoggerRule;
43+
import org.jvnet.hudson.test.recipes.LocalData;
3944
import org.w3c.dom.Element;
4045
import org.w3c.dom.Node;
46+
import static org.hamcrest.MatcherAssert.assertThat;
47+
import static org.hamcrest.Matchers.equalTo;
48+
import static org.hamcrest.Matchers.is;
49+
import static org.hamcrest.Matchers.not;
50+
import static org.hamcrest.Matchers.nullValue;
4151

4252
public final class BulkFlowNodeStorageTest {
4353

4454
@Rule public JenkinsRule r = new JenkinsRule();
55+
@Rule public LoggerRule logger = new LoggerRule().record(RobustReflectionConverter.class, Level.FINE).capture(50);
4556

4657
@Test public void orderOfEntries() throws Exception {
4758
var p = r.createProject(WorkflowJob.class, "p");
@@ -58,4 +69,40 @@ public final class BulkFlowNodeStorageTest {
5869
assertThat(ids, is(IntStream.rangeClosed(2, 43).mapToObj(Integer::toString).collect(Collectors.toList())));
5970
}
6071

72+
@LocalData
73+
@Test public void actionDeserializationShouldBeRobust() throws Exception {
74+
/*
75+
var p = r.createProject(WorkflowJob.class);
76+
p.addProperty(new DurabilityHintJobProperty(FlowDurabilityHint.PERFORMANCE_OPTIMIZED));
77+
p.setDefinition(new CpsFlowDefinition(
78+
"stage('test') {\n" +
79+
" sleep 120\n" +
80+
"}\n", true));
81+
var b = p.scheduleBuild2(0).waitForStart();
82+
Thread.sleep(5*1000);
83+
((CpsFlowExecution) b.getExecution()).getStorage().flush();
84+
*/
85+
var p = r.jenkins.getItemByFullName("test0", WorkflowJob.class);
86+
var b = p.getLastBuild();
87+
// Build is unresumable because the local data was created with PERFORMANCE_OPTIMIZED without a clean shutdown.
88+
r.assertBuildStatus(Result.FAILURE, r.waitForCompletion(b));
89+
// Existing flow nodes should still be preserved though.
90+
var stageBodyStartNode = (StepStartNode) b.getExecution().getNode("4");
91+
assertThat(stageBodyStartNode, not(nullValue()));
92+
var label = stageBodyStartNode.getPersistentAction(LabelAction.class);
93+
assertThat(label.getDisplayName(), equalTo("test"));
94+
}
95+
// Used to create @LocalData for above test:
96+
/*
97+
public static class MyAction extends InvisibleAction implements PersistentAction {
98+
private final String value = "42";
99+
}
100+
@TestExtension("actionDeserializationShouldBeRobust")
101+
public static class MyActionAdder implements GraphListener.Synchronous {
102+
@Override
103+
public void onNewHead(FlowNode node) {
104+
node.addAction(new MyAction());
105+
}
106+
}
107+
*/
61108
}

‎src/test/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamStorageTest.java‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,33 @@
11
package org.jenkinsci.plugins.workflow.support.storage;
22

33

4+
import hudson.model.Result;
5+
import hudson.util.RobustReflectionConverter;
6+
import java.io.File;
7+
import java.util.logging.Level;
48
import org.jenkinsci.plugins.workflow.actions.BodyInvocationAction;
59
import org.jenkinsci.plugins.workflow.actions.LabelAction;
610
import org.jenkinsci.plugins.workflow.actions.TimingAction;
11+
import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
712
import org.jenkinsci.plugins.workflow.graph.AtomNode;
13+
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
814
import org.junit.Assert;
15+
import org.junit.Rule;
916
import org.junit.Test;
10-
11-
import java.io.File;
17+
import org.jvnet.hudson.test.LoggerRule;
18+
import org.jvnet.hudson.test.recipes.LocalData;
19+
import static org.hamcrest.MatcherAssert.assertThat;
20+
import static org.hamcrest.Matchers.equalTo;
21+
import static org.hamcrest.Matchers.not;
22+
import static org.hamcrest.Matchers.nullValue;
1223

1324
/**
1425
* Tries to test the storage engine
1526
*/
1627
public class SimpleXStreamStorageTest extends AbstractStorageTest {
1728

29+
@Rule public LoggerRule logger = new LoggerRule().record(RobustReflectionConverter.class, Level.FINE).capture(50);
30+
1831
@Override
1932
public FlowNodeStorage instantiateStorage(MockFlowExecution exec, File storageDirectory) {
2033
return new SimpleXStreamFlowNodeStorage(exec, storageDirectory);
@@ -72,4 +85,39 @@ public void testDeferWriteAndFlush() throws Exception {
7285
mock2.setStorage(storageAfterRead);
7386
StorageTestUtils.assertNodesMatch(deferredWriteNode, storageAfterRead.getNode(deferredWriteNode.getId()));
7487
}
88+
89+
@LocalData
90+
@Test public void actionDeserializationShouldBeRobust() throws Exception {
91+
/*
92+
var p = j.createProject(WorkflowJob.class);
93+
p.addProperty(new DurabilityHintJobProperty(FlowDurabilityHint.MAX_SURVIVABILITY));
94+
p.setDefinition(new CpsFlowDefinition(
95+
"stage('test') {\n" +
96+
" sleep 120\n" +
97+
"}\n", true));
98+
var b = p.scheduleBuild2(0).waitForStart();
99+
Thread.sleep(5*1000);
100+
((CpsFlowExecution) b.getExecution()).getStorage().flush();
101+
*/
102+
var p = j.jenkins.getItemByFullName("test0", WorkflowJob.class);
103+
var b = p.getLastBuild();
104+
j.assertBuildStatus(Result.SUCCESS, j.waitForCompletion(b));
105+
var stageBodyStartNode = (StepStartNode) b.getExecution().getNode("4");
106+
assertThat(stageBodyStartNode, not(nullValue()));
107+
var label = stageBodyStartNode.getPersistentAction(LabelAction.class);
108+
assertThat(label.getDisplayName(), equalTo("test"));
109+
}
110+
// Used to create @LocalData for above test:
111+
/*
112+
public static class MyAction extends InvisibleAction implements PersistentAction {
113+
private final String value = "42";
114+
}
115+
@TestExtension("actionDeserializationShouldBeRobust")
116+
public static class MyActionAdder implements GraphListener.Synchronous {
117+
@Override
118+
public void onNewHead(FlowNode node) {
119+
node.addAction(new MyAction());
120+
}
121+
}
122+
*/
75123
}

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/build.xml‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?xml version="1.1" encoding="UTF-8"?>
2+
<flow-build plugin="[email protected]">
3+
<actions/>
4+
<queueId>1</queueId>
5+
<timestamp>1714512454761</timestamp>
6+
<startTime>1714512454774</startTime>
7+
<duration>0</duration>
8+
<charset>UTF-8</charset>
9+
<keepLog>false</keepLog>
10+
<execution class="org.jenkinsci.plugins.workflow.cps.CpsFlowExecution">
11+
<result>SUCCESS</result>
12+
<script>stage(&apos;test&apos;) {
13+
sleep 120
14+
}
15+
</script>
16+
<loadedScripts class="map"/>
17+
<durabilityHint>PERFORMANCE_OPTIMIZED</durabilityHint>
18+
<timings class="map">
19+
<entry>
20+
<string>flowNode</string>
21+
<long>3116376</long>
22+
</entry>
23+
<entry>
24+
<string>classLoad</string>
25+
<long>125882830</long>
26+
</entry>
27+
<entry>
28+
<string>run</string>
29+
<long>80228333</long>
30+
</entry>
31+
<entry>
32+
<string>parse</string>
33+
<long>213222375</long>
34+
</entry>
35+
</timings>
36+
<internalCalls class="sorted-set"/>
37+
<sandbox>true</sandbox>
38+
<iota>5</iota>
39+
<head>1:5</head>
40+
<start>2</start>
41+
<done>false</done>
42+
<resumeBlocked>false</resumeBlocked>
43+
</execution>
44+
<completed>false</completed>
45+
<checkouts class="hudson.util.PersistedList"/>
46+
</flow-build>

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Started
2+
ha:////4Er24oWfBBiEN/Wa7id5o9M/o5Uqubl8YTcDNNanS4ZDAAAAoh+LCAAAAAAAAP9tjTEOwjAQBM8BClpKHuFItIiK1krDC0x8GCfWnbEdkooX8TX+gCESFVvtrLSa5wtWKcKBo5UdUu8otU4GP9jS5Mixv3geZcdn2TIl9igbHBs2eJyx4YwwR1SwULBGaj0nRzbDRnX6rmuvydanHMu2V1A5c4MHCFXMWcf8hSnC9jqYxPTz/BXAFEIGsfuclm8zQVqFvQAAAA==[Pipeline] Start of Pipeline
3+
ha:////4P/BX1JLzbsCrvx6yk49claR+ti0ovHPJGy4iulK196/AAAApR+LCAAAAAAAAP9tjTEOwjAUQ3+KOrAycohUghExsUZZOEFIQkgb/d8mKe3EibgadyBQiQlLlmxL1nu+oE4RjhQdby12HpP2vA+jK4lPFLtroIm3dOGaMFGwXNpJkrGnpUrKFhaxClYC1hZ1oOTRZdiIVt1VExS65pxj2Q4CKm8GeAAThZxVzN8yR9jeRpMIf5y/AJj7DGxXvP/86jduZBmjwAAAAA==[Pipeline] stage
4+
ha:////4JdiHwYzioWlo2ROxl2Zlo74y53NN96hzyE6iT+69Eh0AAAApR+LCAAAAAAAAP9tjTEOwjAUQ3+KOrAycoh0gA0xsUZZOEFIQkgb/d8mKe3EibgadyBQiQlLlmxL1nu+oE4RjhQdby12HpP2vA+jK4lPFLtroIm3dOGaMFGwXNpJkrGnpUrKFhaxClYC1hZ1oOTRZdiIVt1VExS65pxj2Q4CKm8GeAAThZxVzN8yR9jeRpMIf5y/AJj7DGxXvP/86jfoP95RwAAAAA==[Pipeline] { (test)
5+
ha:////4IbbNcCvGzKmOkFNSNkP4IRFXNaMMv4VDyWQd8mj5yT1AAAAoh+LCAAAAAAAAP9tjTEOAiEURD9rLGwtPQTbaGWsbAmNJ0AWEZb8zwLrbuWJvJp3kLiJlZNMMm+a93rDOic4UbLcG+wdZu14DKOti0+U+lugiXu6ck2YKRguzSSpM+cFJRUDS1gDKwEbgzpQdmgLbIVXD9UGhba9lFS/o4DGdQM8gYlqLiqVL8wJdvexy4Q/z18BzLEA29ce4gdpL1fxvAAAAA==[Pipeline] sleep
6+
Sleeping for 2 min 0 sec

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log-index‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
1231 5

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/workflow/flowNodeStore.xml‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?xml version="1.1" encoding="UTF-8"?>
2+
<linked-hash-map>
3+
<entry>
4+
<string>2</string>
5+
<Tag>
6+
<node class="org.jenkinsci.plugins.workflow.graph.FlowStartNode" plugin="[email protected]_">
7+
<parentIds/>
8+
<id>2</id>
9+
</node>
10+
<actions>
11+
<org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
12+
<value>42</value>
13+
</org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
14+
<wf.a.TimingAction plugin="[email protected]_">
15+
<startTime>1714512455074</startTime>
16+
</wf.a.TimingAction>
17+
</actions>
18+
</Tag>
19+
</entry>
20+
<entry>
21+
<string>3</string>
22+
<Tag>
23+
<node class="cps.n.StepStartNode" plugin="[email protected]_3a_6988277b_2">
24+
<parentIds>
25+
<string>2</string>
26+
</parentIds>
27+
<id>3</id>
28+
<descriptorId>org.jenkinsci.plugins.workflow.support.steps.StageStep</descriptorId>
29+
</node>
30+
<actions>
31+
<s.a.LogStorageAction/>
32+
<cps.a.ArgumentsActionImpl plugin="[email protected]_3a_6988277b_2">
33+
<arguments>
34+
<entry>
35+
<string>name</string>
36+
<string>test</string>
37+
</entry>
38+
</arguments>
39+
<sensitiveVariables/>
40+
<isUnmodifiedBySanitization>true</isUnmodifiedBySanitization>
41+
</cps.a.ArgumentsActionImpl>
42+
<wf.a.TimingAction plugin="[email protected]_">
43+
<startTime>1714512455144</startTime>
44+
</wf.a.TimingAction>
45+
<org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
46+
<value>42</value>
47+
</org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
48+
</actions>
49+
</Tag>
50+
</entry>
51+
<entry>
52+
<string>4</string>
53+
<Tag>
54+
<node class="cps.n.StepStartNode" plugin="[email protected]_3a_6988277b_2">
55+
<parentIds>
56+
<string>3</string>
57+
</parentIds>
58+
<id>4</id>
59+
<descriptorId>org.jenkinsci.plugins.workflow.support.steps.StageStep</descriptorId>
60+
</node>
61+
<actions>
62+
<wf.a.BodyInvocationAction plugin="[email protected]_"/>
63+
<wf.a.LabelAction plugin="[email protected]_">
64+
<displayName>test</displayName>
65+
</wf.a.LabelAction>
66+
<wf.a.TimingAction plugin="[email protected]_">
67+
<startTime>1714512455148</startTime>
68+
</wf.a.TimingAction>
69+
<org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
70+
<value>42</value>
71+
</org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
72+
</actions>
73+
</Tag>
74+
</entry>
75+
<entry>
76+
<string>5</string>
77+
<Tag>
78+
<node class="cps.n.StepAtomNode" plugin="[email protected]_3a_6988277b_2">
79+
<parentIds>
80+
<string>4</string>
81+
</parentIds>
82+
<id>5</id>
83+
<descriptorId>org.jenkinsci.plugins.workflow.steps.SleepStep</descriptorId>
84+
</node>
85+
<actions>
86+
<cps.a.ArgumentsActionImpl plugin="[email protected]_3a_6988277b_2">
87+
<arguments>
88+
<entry>
89+
<string>time</string>
90+
<long>120</long>
91+
</entry>
92+
</arguments>
93+
<sensitiveVariables/>
94+
<isUnmodifiedBySanitization>true</isUnmodifiedBySanitization>
95+
</cps.a.ArgumentsActionImpl>
96+
<wf.a.TimingAction plugin="[email protected]_">
97+
<startTime>1714512455155</startTime>
98+
</wf.a.TimingAction>
99+
<org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
100+
<value>42</value>
101+
</org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
102+
<s.a.LogStorageAction/>
103+
</actions>
104+
</Tag>
105+
</entry>
106+
</linked-hash-map>

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/legacyIds‎

Whitespace-only changes.

‎src/test/resources/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/permalinks‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
lastSuccessfulBuild -1

0 commit comments

Comments
 (0)