Notify GraphListener extensions prior to listeners added via FlowExecution.addListener#1052
Conversation
dwnusbaum
changed the title
GraphListeners after GraphListener extensionsGraphListener.ordinal method
dwnusbaum
changed the title
GraphListener.ordinal methodGraphListener extensions prior to listeners added via FlowExecution.addListener
| } | ||
| execution.flowStartNodeActions.clear(); | ||
| } // may be unset from loadProgramFailed | ||
| n.addAction(new TimingAction()); |
There was a problem hiding this comment.
We handle TimingAction here not just to be able to clean up GraphL in jenkinsci/workflow-job-plugin#548, but so that listeners in plugins like datadog and github-autostatus (and probably others) which expect TimingAction to be present when they run continue to work even for FlowStartNode. They should already work fine for all other nodes, which go through setNewHead down below, which has been adding TimingAction since #188.
| listeners = new CopyOnWriteArrayList<>(); | ||
| } | ||
| listeners.add(listener); | ||
| listeners.add(0, listener); |
There was a problem hiding this comment.
It seemed preferable to handle the reversal of this list when listeners are added rather than each time listeners are notified. In practice I think this list will normally only contain WorkflowRun$GraphL and WorkflowRun$NodePrintListener, although WorkflowRun$FailOnLoadListener will be added temporarily.
| var p = r.createProject(WorkflowJob.class); | ||
| p.setDefinition(new CpsFlowDefinition("echo 'test'", true)); | ||
| var b = r.buildAndAssertSuccess(p); | ||
| await().until(() -> listener.done); |
There was a problem hiding this comment.
With the current code, this await is unnecessary, since the listener always runs before the build completes, but you need the await to reproduce the problematic case when reverting the changes to src/main, since otherwise the build (and thus the test) complete before the listener even runs.
|
@jglick Do you think there is still value in doing this even though it doesn't actually matter for any known |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
It seems generally desirable, so if you think this is safe to do, we may as well.
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
|
Ok, I'll try to run it through the PCT first. |
|
I ran the PCT (using a system internal to CloudBees) and things looked ok, so I'll merge this. |
2 participants
See also jenkinsci/workflow-job-plugin#548, which can only be safely merged and released after this PR.
While investigating whether I could change a synchronous
GraphListenerto an asynchronous one in jenkinsci/opentelemetry-plugin#700, I noticed thatWorklowRun.GraphLcurrently always runs beforeGraphListenerextensions. This means that asynchronous listeners may observe nodes that occur towards the end of the build after the build has already completed, which seems undesirable.This PR adjusts the listener notification order so that
GraphListenerextensions are handled first, and then listeners added viaFlowExecution.addListenerare notified in the reverse order that they were added. This PR also starts settingTimingActiondirectly onFlowStartNodeso that timing is always set on all nodes for all listeners and we can clean up some logic inWorkflowRun$GraphL, see jenkinsci/workflow-job-plugin#548.(Earlier, I also considered jenkinsci/workflow-api-plugin#410 and jenkinsci/workflow-job-plugin#547, but those more complicated changes seem to be unnecessary if we just handle
TimingActionforFlowStartNodehere).Given that the only known issue with
Workflow$GraphLcompleting the build before other async listeners run is with a hypothetical change toopentelemetrythat cannot happen for other unrelated reasons, I am not sure if we actually care about going through with this change, but I wanted to look into it just in case it matters for other listeners in the future.Testing done
See new automated tests.
Submitter checklist