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

GraphListener does not receive FlowStartNode

    XMLWordPrintable

    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

          t8ch Thomas Weißschuh created issue -
          svanoort Sam Van Oort made changes -
          Field Original Value New Value
          Assignee Sam Van Oort [ svanoort ]
          svanoort Sam Van Oort made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          svanoort Sam Van Oort added a comment -

          PR with fix + test. 

          Show
          svanoort Sam Van Oort added a comment - PR with fix + test. 
          svanoort Sam Van Oort made changes -
          Remote Link This issue links to "Workflow-cps #232 (Web Link)" [ 21134 ]
          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.
          dnusbaum Devin Nusbaum made changes -
          Released As workflow-cps 2.63
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Resolved [ 5 ]
          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()); } }); } }
          t8ch Thomas Weißschuh made changes -
          Assignee Sam Van Oort [ svanoort ] Devin Nusbaum [ dnusbaum ]
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          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.
          allanlewis_youview Allan Lewis made changes -
          Assignee Devin Nusbaum [ dnusbaum ] Allan Lewis [ allanlewis_youview ]
          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.
          allanlewis_youview Allan Lewis made changes -
          Assignee Allan Lewis [ allanlewis_youview ] Sam Van Oort [ svanoort ]
          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?
          t8ch Thomas Weißschuh made changes -
          Assignee Sam Van Oort [ svanoort ] Devin Nusbaum [ dnusbaum ]
          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.
          dnusbaum Devin Nusbaum made changes -
          Assignee Devin Nusbaum [ dnusbaum ]
          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?
          dnusbaum Devin Nusbaum made changes -
          Remote Link This issue links to "jenkinsci/workflow-job-plugin#129 (Web Link)" [ 23421 ]
          dnusbaum Devin Nusbaum made changes -
          Remote Link This issue links to "jenkinsci/workflow-api-plugin#92 (Web Link)" [ 23422 ]
          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.
          dnusbaum Devin Nusbaum made changes -
          Released As workflow-cps 2.63 workflow-cps 2.63, workflow-api 2.36, workflow-job 2.34
          Assignee Thomas Weißschuh [ t8ch ]
          Resolution Fixed [ 1 ]
          Status Reopened [ 4 ] Resolved [ 5 ]

            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: