Flowscanner reimplement jenkins28119 error info #3
Conversation
| return (a != null && stackTraces.contains(Functions.printThrowable(a.getError()))); | ||
| } | ||
| }; | ||
| return new FlowScanner.DepthFirstScanner().findFirstMatch(getExecution(), threwException); |
There was a problem hiding this comment.
Once the ForkScanner is complete, it should be far more efficient for this case, because:
- It doesn't have to keep a HashSet of all visited FlowNodes (just a stack with depth related to the number of levels of parallel branches).
- DepthFirstScanner will walk to the start of the flow, if the failure occurs in any branch branch of a parallel block beyond the first. ForkScanner will only walk the up to the start of the parallel block containing the failure (and then go through the branches there).
Technically the completely optimal case here is probably to jump over blocks without an ErrorAction (if we can guarantee they're attached to all the BlockEndNodes where failures happen) AND to do a proper breadth-first search.
There was a problem hiding this comment.
DepthFirstScanner is a top-level type in the current upstream PR.
For a PR downstream of an evolving API PR, it is best to use timestamped snapshots in the Maven dependency, rather than 2.1-SNAPSHOT which is vague.
There was a problem hiding this comment.
Performance is not a significant concern for this PR, because this is code run just once per step execution, typically once per build, and from the CPS VM thread. Obviously if you have some system for indexing nodes using specific action classes, that would be helpful as you could go straight to those with ErrorAction.
I did not follow the discussion about ForkScanner here. At any rate, for purposes of this PR we are looking for any FlowNode in the build with an ErrorAction, so long as it is not a BlockEndNode. Possibly we really want to check also BlockEndNodes, to handle errors thrown only during cleanup from build wrappers and the like, but give them a lower priority. The issue is that ErrorAction gets attached to end nodes of blocks which are exited by an uncaught exception, so we want to find the “first” node with the matching action: i.e., giving preference to the nested node. So it would also suffice to do a walk forward in time, rather than backward as I presume DepthFirstScanner does. Seems there is no such scanner yet?
| for (Throwable t = error; t != null; t = t.getCause()) { | ||
| stackTraces.add(Functions.printThrowable(t)); | ||
| } | ||
| Predicate<FlowNode> threwException = new Predicate<FlowNode>() { |
There was a problem hiding this comment.
I would inline this variable. Just make sure it is safe for lambda conversion when we switch to -source 8. Most likely this is satisfied by just verifying that there are no method overloads which vary only in the declared type of the SAM they accept. (This is where we screwed up with ACL.impersonate.)
Modifies #2 to use the new flow scanners, supplying predicates.
Depends on jenkinsci/workflow-api-plugin#2
@jglick This is the first of the implementations with the new FlowScanners. Would be happy to resubmit as a PR against your fork if you prefer.