Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-52189

GraphListener does not receive FlowStartNode

    Details

    • Similar Issues:
    • Released As:
      workflow-cps 2.63, workflow-api 2.36, workflow-job 2.34

      Description

      Classes implementing org.jenkinsci.plugins.workflow.flow.GraphListener are not informed about FlowStartNode s at execution start.

      All other types of FlowNode are handled.

      (Including the matching FlowEndNode)

        Attachments

          Activity

          Hide
          svanoort Sam Van Oort added a comment -

          PR with fix + test. 

          Show
          svanoort Sam Van Oort added a comment - PR with fix + test. 
          Hide
          dnusbaum Devin Nusbaum added a comment -

          A fix for this issue was released in version 2.63 of Pipeline Groovy Plugin.

          Show
          dnusbaum Devin Nusbaum added a comment - A fix for this issue was released in version 2.63 of Pipeline Groovy Plugin.
          Hide
          t8ch Thomas Weißschuh added a comment - - edited

          Another point (also raised on the PR):

          Should it be possible to use the instantiation based usage of GraphListener (as
          explained by its javadoc?

          Something like the following test:

             @Issue("JENKINS-52189")
              @Test
              public void notifyFlowStartNodeViaFlowExecutionListener() {
                  story.then(s->{
                      WorkflowJob j = jenkins().createProject(WorkflowJob.class, "bob");
                      j.setDefinition(new CpsFlowDefinition("echo 'I did a thing'", true));
                      WorkflowRun r = story.j.buildAndAssertSuccess(j);
                      FlowStartNodeFlowExectionListener listener = jenkins().getExtensionList(
                          FlowStartNodeFlowExectionListener.class).get(0);
                      assertThat(listener.heads, Matchers.greaterThan(1));
                      assertThat(listener.execNames, Matchers.contains(r.getExecution().toString()));
                  });
              }
          
              @TestExtension("notifyFlowStartNodeViaFlowExecutionListener")
              public static class FlowStartNodeFlowExectionListener extends FlowExecutionListener {
                  int heads = 0;
                  final List<String> execNames = new ArrayList<>();
          
                  @Override
                  public void onRunning(@Nonnull FlowExecution execution) {
                      execution.addListener(node -> {
                          heads++;
                          if (node instanceof FlowStartNode) {
                              execNames.add(node.getExecution().toString());
                          }
                      });
                  }
              }
          
          Show
          t8ch Thomas Weißschuh added a comment - - edited Another point (also raised on the PR): Should it be possible to use the instantiation based usage of GraphListener (as explained by its javadoc? Something like the following test: @Issue( "JENKINS-52189" ) @Test public void notifyFlowStartNodeViaFlowExecutionListener() { story.then(s->{ WorkflowJob j = jenkins().createProject(WorkflowJob.class, "bob" ); j.setDefinition( new CpsFlowDefinition( "echo 'I did a thing' " , true )); WorkflowRun r = story.j.buildAndAssertSuccess(j); FlowStartNodeFlowExectionListener listener = jenkins().getExtensionList( FlowStartNodeFlowExectionListener.class).get(0); assertThat(listener.heads, Matchers.greaterThan(1)); assertThat(listener.execNames, Matchers.contains(r.getExecution().toString())); }); } @TestExtension( "notifyFlowStartNodeViaFlowExecutionListener" ) public static class FlowStartNodeFlowExectionListener extends FlowExecutionListener { int heads = 0; final List< String > execNames = new ArrayList<>(); @Override public void onRunning(@Nonnull FlowExecution execution) { execution.addListener(node -> { heads++; if (node instanceof FlowStartNode) { execNames.add(node.getExecution().toString()); } }); } }
          Hide
          dnusbaum Devin Nusbaum added a comment -

          Thomas Weißschuh I don't know. If you have a use case where you need to use FlowExecutionListener.onRunning to add a listener and cannot use a global extension, then perhaps better to file a separate bug for that specific case. Otherwise, maybe we can just update the docs for FlowExecutionListener#onRunning to mention that if you add a listener in that method it will not see FlowStartNodes.

          Show
          dnusbaum Devin Nusbaum added a comment - Thomas Weißschuh I don't know. If you have a use case where you need to use FlowExecutionListener.onRunning to add a listener and cannot use a global extension, then perhaps better to file a separate bug for that specific case. Otherwise, maybe we can just update the docs for FlowExecutionListener#onRunning to mention that if you add a listener in that method it will not see FlowStartNodes.
          Hide
          t8ch Thomas Weißschuh added a comment -

          My current code is nicer to implement with customly created listeners. I can rewrite it to use the global one but wanted to ask before, as maybe it should actually work.

          My FlowExecutionListener creates a GraphListener for every new Flow (and stores it).
          The following snippet works around the currently missing functionality by triggering the creation of the GraphListener "manually":

          import hudson.Extension;
          import org.jenkinsci.plugins.workflow.flow.GraphListener;
          import org.jenkinsci.plugins.workflow.graph.FlowNode;
          import org.jenkinsci.plugins.workflow.graph.FlowStartNode;
          
          /*
           * This class works around https://issues.jenkins-ci.org/browse/JENKINS-52189
           * Especially the part where GraphListeners attached during FlowExecutionListener.onRunning() do not receive the FlowStartNode.
           * As the per instance logic is nicer to reason about and may work properly in a future version of Jenkins,
           * this class will be the bridge until then.
           */
          @Extension
          public class WorkaroundGraphListener implements GraphListener, GraphListener.Synchronous {
          
              @Override
              public void onNewHead(FlowNode node) {
                  final OTFlowExecutionListener flowExecutionListener = OTFlowExecutionListener.get();
                  if (node instanceof FlowStartNode) {
                      flowExecutionListener.getListener(node.getExecution()).onNewHead(node);
                  }
              }
          }
          
          Show
          t8ch Thomas Weißschuh added a comment - My current code is nicer to implement with customly created listeners. I can rewrite it to use the global one but wanted to ask before, as maybe it should actually work. My FlowExecutionListener creates a GraphListener for every new Flow (and stores it). The following snippet works around the currently missing functionality by triggering the creation of the GraphListener "manually": import hudson.Extension; import org.jenkinsci.plugins.workflow.flow.GraphListener; import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.graph.FlowStartNode; /* * This class works around https: //issues.jenkins-ci.org/browse/JENKINS-52189 * Especially the part where GraphListeners attached during FlowExecutionListener.onRunning() do not receive the FlowStartNode. * As the per instance logic is nicer to reason about and may work properly in a future version of Jenkins, * this class will be the bridge until then. */ @Extension public class WorkaroundGraphListener implements GraphListener, GraphListener.Synchronous { @Override public void onNewHead(FlowNode node) { final OTFlowExecutionListener flowExecutionListener = OTFlowExecutionListener.get(); if (node instanceof FlowStartNode) { flowExecutionListener.getListener(node.getExecution()).onNewHead(node); } } }
          Hide
          dnusbaum Devin Nusbaum added a comment -

          Thomas Weißschuh If you can figure out a clean way to make it work and submit a PR, then I'd be happy to review it. I don't think there is any reason why we wouldn't want it to work, just that the most obvious implementation happens to not work for that use case.

          Show
          dnusbaum Devin Nusbaum added a comment - Thomas Weißschuh If you can figure out a clean way to make it work and submit a PR, then I'd be happy to review it. I don't think there is any reason why we wouldn't want it to work, just that the most obvious implementation happens to not work for that use case.
          Hide
          t8ch Thomas Weißschuh added a comment -

          Allan Lewis you assigned this to yourself, are you planning on working on this?

          Otherwise I may find the time to take a shot at it.

          Show
          t8ch Thomas Weißschuh added a comment - Allan Lewis you assigned this to yourself, are you planning on working on this? Otherwise I may find the time to take a shot at it.
          Hide
          allanlewis_youview Allan Lewis added a comment -

          Sorry, Thomas Weißschuh, I did that by mistake! I probably pressed 'i' while this ticket was focused.

          Show
          allanlewis_youview Allan Lewis added a comment - Sorry, Thomas Weißschuh , I did that by mistake! I probably pressed 'i' while this ticket was focused.
          Hide
          t8ch Thomas Weißschuh added a comment -

          Devin Nusbaum so the easy way would be to just fire the listeners before the execution
          This however would break the behaviour of the `FlowExecution.onRunning()` method which is documented to receive an already started listener.
          (The testsuite however passes)

          We could also introduce a new method `FlowExecution.beforeRunning()` (or `onCreated()`) that fires before the flow is actually started.

          WDYT?

          Show
          t8ch Thomas Weißschuh added a comment - Devin Nusbaum so the easy way would be to just fire the listeners before the execution This however would break the behaviour of the `FlowExecution.onRunning()` method which is documented to receive an already started listener . (The testsuite however passes) We could also introduce a new method `FlowExecution.beforeRunning()` (or `onCreated()`) that fires before the flow is actually started. WDYT?
          Hide
          dnusbaum Devin Nusbaum added a comment -

          Thomas Weißschuh I guess introducing a new method `onCreated()` seems better in case some code in the wild relies on the documented behavior. Feel free to file a PR, ideally linked to an update to some other plugin that wants to use the new method.

          Show
          dnusbaum Devin Nusbaum added a comment - Thomas Weißschuh I guess introducing a new method `onCreated()` seems better in case some code in the wild relies on the documented behavior. Feel free to file a PR, ideally linked to an update to some other plugin that wants to use the new method.
          Hide
          t8ch Thomas Weißschuh added a comment -

          Devin Nusbaum: https://github.com/jenkinsci/workflow-job-plugin/pull/129
          Unfortunately the plugin that needs it is not (yet?) public.

          Show
          t8ch Thomas Weißschuh added a comment - Devin Nusbaum : https://github.com/jenkinsci/workflow-job-plugin/pull/129 Unfortunately the plugin that needs it is not (yet?) public.
          Hide
          mawinter69 Markus Winter added a comment -

          Having the same issue with the missed FlowStartNode

          Any idea when the fixes will be released?

          Show
          mawinter69 Markus Winter added a comment - Having the same issue with the missed FlowStartNode Any idea when the fixes will be released?
          Hide
          dnusbaum Devin Nusbaum added a comment -

          The new FlowExecutionListener.onCreated method was released in Pipeline API Plugin 2.36, and is activated by Pipeline Job Plugin 2.34.

          Show
          dnusbaum Devin Nusbaum added a comment - The new FlowExecutionListener.onCreated method was released in Pipeline API Plugin 2.36, and is activated by Pipeline Job Plugin 2.34.

            People

            • Assignee:
              t8ch Thomas Weißschuh
              Reporter:
              t8ch Thomas Weißschuh
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: