From 52bb5b7a4b40eadcbcb78abb09767017bc628773 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 22 Jun 2023 12:55:02 -0400 Subject: [PATCH 1/3] Add test demonstrating that ForkScanner visits parallel step start nodes more than once in some cases --- .../graphanalysis/ForkScannerTest.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java index 53fd0825..e98ed370 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java @@ -992,4 +992,42 @@ public void testParallelCorrectEndNodeForVisitor() throws Exception { testParallelFindsLast(jobPauseSecond, "wait2"); testParallelFindsLast(jobPauseMiddle, "wait3"); } + + @Test + public void parallelInParallelVisitsOuterParallelStartTwice() throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class); + p.setDefinition(new CpsFlowDefinition( + "parallel(\n" + + " outerA: {\n" + + " semaphore('outerA')\n" + + " },\n" + + " outerB: {\n" + + " echo('outerB')\n" + + " },\n" + + " outerC: {\n" + + " parallel(\n" + + " innerA: {\n" + + " semaphore('innerA')\n" + + " },\n" + + " innerB: {\n" + + " echo('innerB')\n" + + " }\n" + + " )\n" + + " }\n" + + ")\n", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + SemaphoreStep.waitForStart("outerA/1", b); + SemaphoreStep.waitForStart("innerA/1", b); + ForkScanner scanner = new ForkScanner(); + scanner.setup(b.getExecution().getCurrentHeads()); + Set visited = new HashSet<>(); + for (FlowNode node : scanner) { + if (!visited.add(node.getId())) { + Assert.fail("Node " + node.getId() + " was visited more than once"); + } + } + SemaphoreStep.success("outerA/1", null); + SemaphoreStep.success("innerA/1", null); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + } } From 47584995fcd099abc0c968487776022ab7709dd1 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 22 Jun 2023 13:13:11 -0400 Subject: [PATCH 2/3] Enhance test to demonstrate that many nodes are visited more than once --- .../graphanalysis/ForkScannerTest.java | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java index e98ed370..ccf0d433 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java @@ -59,6 +59,9 @@ import java.util.function.Predicate; import java.util.stream.Collectors; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + /** * Tests for internals of ForkScanner */ @@ -997,35 +1000,36 @@ public void testParallelCorrectEndNodeForVisitor() throws Exception { public void parallelInParallelVisitsOuterParallelStartTwice() throws Exception { WorkflowJob p = r.createProject(WorkflowJob.class); p.setDefinition(new CpsFlowDefinition( - "parallel(\n" + - " outerA: {\n" + - " semaphore('outerA')\n" + - " },\n" + - " outerB: {\n" + - " echo('outerB')\n" + - " },\n" + - " outerC: {\n" + - " parallel(\n" + - " innerA: {\n" + - " semaphore('innerA')\n" + - " },\n" + - " innerB: {\n" + - " echo('innerB')\n" + - " }\n" + - " )\n" + - " }\n" + - ")\n", true)); + "stage('MyStage') {\n" + + " parallel(\n" + + " outerA: {\n" + + " semaphore('outerA')\n" + + " },\n" + + " outerB: {\n" + + " echo('outerB')\n" + + " },\n" + + " outerC: {\n" + + " parallel(\n" + + " innerA: {\n" + + " semaphore('innerA')\n" + + " },\n" + + " innerB: {\n" + + " echo('innerB')\n" + + " }\n" + + " )\n" + + " }\n" + + " )\n" + + "}\n", true)); WorkflowRun b = p.scheduleBuild2(0).waitForStart(); SemaphoreStep.waitForStart("outerA/1", b); SemaphoreStep.waitForStart("innerA/1", b); ForkScanner scanner = new ForkScanner(); scanner.setup(b.getExecution().getCurrentHeads()); - Set visited = new HashSet<>(); + List visited = new ArrayList<>(); for (FlowNode node : scanner) { - if (!visited.add(node.getId())) { - Assert.fail("Node " + node.getId() + " was visited more than once"); - } + visited.add(node.getId()); } + assertThat("Each FlowNode should be visited only once " + visited, visited.size(), equalTo(new HashSet<>(visited).size())); SemaphoreStep.success("outerA/1", null); SemaphoreStep.success("innerA/1", null); r.assertBuildStatusSuccess(r.waitForCompletion(b)); From f1242e7cd8f9b0695f687bcbe3b0dd78f3c80920 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 22 Jun 2023 15:48:42 -0400 Subject: [PATCH 3/3] Order nested parallel branches from innermost to outermost to avoid revisiting nodes --- .../workflow/graphanalysis/ForkScanner.java | 12 +++++- .../graphanalysis/ForkScannerTest.java | 37 ++++++++----------- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java b/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java index 82d5da13..d0f9a07d 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java @@ -48,6 +48,7 @@ import java.util.List; import java.util.ListIterator; import java.util.Set; +import java.util.stream.Collectors; /** * Scanner that will scan down all forks when we hit parallel blocks before continuing, but generally runs in linear order @@ -443,8 +444,15 @@ ArrayDeque leastCommonAncestor(@NonNull final Set throw new IllegalStateException("No least common ancestor found from " + heads); } - // If we hit issues with the ordering of blocks by depth, apply a sorting to the parallels by depth - return convertForksToBlockStarts(parallelForks); + // The result must be in reverse topological order, i.e. inner branches must be visited before outer branches. + return convertForksToBlockStarts(parallelForks).stream().sorted((pbs1, pbs2) -> { + if (pbs1.forkStart.getEnclosingBlocks().contains(pbs2.forkStart)) { + return -1; + } else if (pbs2.forkStart.getEnclosingBlocks().contains(pbs1.forkStart)) { + return 1; + } + return 0; + }).collect(Collectors.toCollection(ArrayDeque::new)); } @Override diff --git a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java index ccf0d433..65722748 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScannerTest.java @@ -59,9 +59,6 @@ import java.util.function.Predicate; import java.util.stream.Collectors; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; - /** * Tests for internals of ForkScanner */ @@ -997,23 +994,23 @@ public void testParallelCorrectEndNodeForVisitor() throws Exception { } @Test - public void parallelInParallelVisitsOuterParallelStartTwice() throws Exception { + public void inProgressParallelInParallel() throws Exception { WorkflowJob p = r.createProject(WorkflowJob.class); p.setDefinition(new CpsFlowDefinition( - "stage('MyStage') {\n" + - " parallel(\n" + - " outerA: {\n" + - " semaphore('outerA')\n" + - " },\n" + - " outerB: {\n" + - " echo('outerB')\n" + + "stage('MyStage') {\n" + // 3, 4 + " parallel(\n" + // 5 + " outerA: {\n" + // 8 + " semaphore('outerA')\n" + // 11 " },\n" + - " outerC: {\n" + - " parallel(\n" + - " innerA: {\n" + - " semaphore('innerA')\n" + + " outerB: {\n" + // 9 + " echo('outerB')\n" + // 12 + " },\n" + // 13 + " outerC: {\n" + // 10 + " parallel(\n" + // 14 + " innerA: {\n" + // 16 + " semaphore('innerA')\n" + // 18 " },\n" + - " innerB: {\n" + + " innerB: {\n" + // 17 " echo('innerB')\n" + " }\n" + " )\n" + @@ -1024,14 +1021,10 @@ public void parallelInParallelVisitsOuterParallelStartTwice() throws Exception { SemaphoreStep.waitForStart("outerA/1", b); SemaphoreStep.waitForStart("innerA/1", b); ForkScanner scanner = new ForkScanner(); - scanner.setup(b.getExecution().getCurrentHeads()); - List visited = new ArrayList<>(); - for (FlowNode node : scanner) { - visited.add(node.getId()); - } - assertThat("Each FlowNode should be visited only once " + visited, visited.size(), equalTo(new HashSet<>(visited).size())); + sanityTestIterationAndVisiter(b.getExecution().getCurrentHeads()); SemaphoreStep.success("outerA/1", null); SemaphoreStep.success("innerA/1", null); r.assertBuildStatusSuccess(r.waitForCompletion(b)); + sanityTestIterationAndVisiter(b.getExecution().getCurrentHeads()); } }