diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 71019d3a6..f4831addc 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -323,6 +323,9 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { } } + /** + * Holds listeners added via {@link #addListener} in the reverse order that they were added. + */ private transient List listeners; /** @@ -1229,7 +1232,7 @@ public void addListener(GraphListener listener) { if (listeners == null) { listeners = new CopyOnWriteArrayList<>(); } - listeners.add(listener); + listeners.add(0, listener); } @Override public void removeListener(GraphListener listener) { @@ -1527,12 +1530,13 @@ private static void cleanUpClassHelperCache(@NonNull Class clazz) throws Exce } List getListenersToRun() { - List l = new ArrayList<>(); - + // Listeners from extensions always come first, ordered by `Extension.ordinal`. Listeners added via + // `addListener` are then notified in the reverse order that they were added (see `addListener`) so that the + // `build-finalizing WorkflowRun$GraphL` always runs last. + List l = new ArrayList<>(ExtensionList.lookup(GraphListener.class)); if (listeners != null) { l.addAll(listeners); } - l.addAll(ExtensionList.lookup(GraphListener.class)); return l; } diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java index db4d2026b..3e0e00a4d 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java @@ -111,6 +111,7 @@ void newStartNode(FlowStartNode n) throws IOException { } execution.flowStartNodeActions.clear(); } // may be unset from loadProgramFailed + n.addAction(new TimingAction()); synchronized (execution) { this.head = execution.startNodes.push(n); } diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java index e2a1bf65a..7a12bff11 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/GraphListenerTest.java @@ -1,19 +1,26 @@ package org.jenkinsci.plugins.workflow; +import static org.awaitility.Awaitility.await; +import static org.hamcrest.Matchers.is; + +import hudson.ExtensionList; import hudson.model.Run; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution; import org.jenkinsci.plugins.workflow.flow.GraphListener; +import org.jenkinsci.plugins.workflow.graph.FlowEndNode; import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Assert; -import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ErrorCollector; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.LoggerRule; import org.jvnet.hudson.test.TestExtension; +import java.io.IOException; import java.io.Serializable; import java.util.List; import java.util.Random; @@ -22,12 +29,15 @@ public class GraphListenerTest { - @ClassRule - public static JenkinsRule r = new JenkinsRule(); + @Rule + public JenkinsRule r = new JenkinsRule(); @Rule public LoggerRule logging = new LoggerRule(); + @Rule + public ErrorCollector errors = new ErrorCollector(); + private static final String LOG_MESSAGE = "some problem here"; @Issue("JENKINS-54890") @@ -47,7 +57,7 @@ public void listener() Assert.assertTrue( "cannot find listener exception message", found > 0 ); } - @TestExtension + @TestExtension("listener") public static class TestGraphListener implements GraphListener, Serializable { @@ -61,4 +71,33 @@ public void onNewHead( FlowNode flowNode ) throw new NullPointerException( LOG_MESSAGE ); } } + + @Test + public void listenersRunBeforeBuildCompletion() throws Exception { + var listener = ExtensionList.lookupSingleton(CheckBuildCompletionListener.class); + listener.errors = errors; + var p = r.createProject(WorkflowJob.class); + p.setDefinition(new CpsFlowDefinition("echo 'test'", true)); + var b = r.buildAndAssertSuccess(p); + await().until(() -> listener.done); + } + + @TestExtension("listenersRunBeforeBuildCompletion") + public static class CheckBuildCompletionListener implements GraphListener { + private ErrorCollector errors; + private boolean done; + + @Override + public void onNewHead(FlowNode node) { + if (node instanceof FlowEndNode) { + try { + var b = (WorkflowRun) node.getExecution().getOwner().getExecutable(); + errors.checkThat("Listeners should always run before build completion", b.isLogUpdated(), is(true)); + } catch (IOException e) { + errors.addError(e); + } + done = true; + } + } + } } diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java index 2533155ce..bc8a34938 100644 --- a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java +++ b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java @@ -91,6 +91,7 @@ import org.htmlunit.WebRequest; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted; +import org.jenkinsci.plugins.workflow.actions.TimingAction; import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.TimingFlowNodeStorage; import org.jenkinsci.plugins.workflow.cps.GroovySourceFileAllowlist.DefaultAllowlist; import org.jenkinsci.plugins.workflow.flow.FlowExecution; @@ -1007,4 +1008,18 @@ public boolean takesImplicitBlockArgument() { } } + @Test public void timingActionAlwaysAdded() throws Throwable { + sessions.then(r -> { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("parallel(one: { stage('1') { echo '1' } }, two: { echo '2' })", true)); + WorkflowRun b = r.buildAndAssertSuccess(p); + var nodesWithoutTiming = new DepthFirstScanner() + .allNodes(b.getExecution()) + .stream() + .filter(n -> n.getPersistentAction(TimingAction.class) == null) + .toList(); + assertThat(nodesWithoutTiming, empty()); + }); + } + }