Skip to content

Flowscanner reimplement jenkins28119 error info #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
svanoort wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Flowscanner reimplement jenkins28119 error info #3

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf118de
[JENKINS-25894] Pick up fix from https://github.com/jenkinsci/workflo…
jglick Apr 13, 2016
d8cf0e2
Sketch of ErrorInfoStep.
jglick Apr 13, 2016
81e422d
FindBugs
jglick Apr 13, 2016
5b5d33d
Use the new FlowScanner implementations for locating error/log info
svanoort Apr 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Use the new FlowScanner implementations for locating error/log info
  • Loading branch information
@svanoort
svanoort committed Apr 27, 2016
commit 5b5d33d6eca48fe34ff1a84cc7710b46900ffee7
41 changes: 20 additions & 21 deletions src/main/java/org/jenkinsci/plugins/workflow/steps/ErrorInfoStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

package org.jenkinsci.plugins.workflow.steps;

import com.google.common.base.Predicate;
import com.google.inject.Inject;
import hudson.AbortException;
import hudson.Extension;
Expand All @@ -42,9 +43,8 @@
import org.jenkinsci.plugins.workflow.flow.FlowExecution;
import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
import org.jenkinsci.plugins.workflow.graph.FlowGraphWalker;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.graph.FlowNodeSerialWalker;
import org.jenkinsci.plugins.workflow.graph.FlowScanner;
import org.kohsuke.stapler.DataBoundConstructor;

/**
Expand Down Expand Up @@ -115,22 +115,21 @@ private FlowExecution getExecution() throws IOException {
* so we compare by stack trace instead.
*/
private @CheckForNull FlowNode getNode() throws IOException {
Set<String> stackTraces = new HashSet<>();
final Set<String> stackTraces = new HashSet<>();
for (Throwable t = error; t != null; t = t.getCause()) {
stackTraces.add(Functions.printThrowable(t));
}
for (FlowNode n : new FlowGraphWalker(getExecution())) {
if (n instanceof BlockEndNode) {
continue; // look for the thing it is enclosing
}
ErrorAction a = n.getAction(ErrorAction.class);
if (a != null) {
if (stackTraces.contains(Functions.printThrowable(a.getError()))) {
return n;
Predicate<FlowNode> threwException = new Predicate<FlowNode>() {
Copy link
Member

@jglick jglick May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@Override
public boolean apply(FlowNode input) {
if (input instanceof BlockEndNode) {
return false;
}
ErrorAction a = input.getAction(ErrorAction.class);
return (a != null && stackTraces.contains(Functions.printThrowable(a.getError())));
}
}
return null;
};
return new FlowScanner.DepthFirstScanner().findFirstMatch(getExecution(), threwException);
Copy link
Member Author

@svanoort svanoort Apr 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the ForkScanner is complete, it should be far more efficient for this case, because:

  1. 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).
  2. 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.

Copy link
Member

@jglick jglick May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jglick jglick May 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

@Whitelisted
Expand Down Expand Up @@ -172,15 +171,15 @@ private FlowExecution getExecution() throws IOException {
public @CheckForNull String getLogURL() throws IOException {
FlowNode n = getNode();
if (n != null) {
for (FlowNode n2 : new FlowNodeSerialWalker(n)) {
LogAction a = n2.getAction(LogAction.class);
if (a != null) {
String u = Jenkins.getActiveInstance().getRootUrl();
if (u == null) {
u = "http://jenkins/"; // placeholder
}
return u + n2.getUrl() + a.getUrlName();
FlowNode logNode = new FlowScanner.LinearBlockHoppingScanner()
.findFirstMatch(n,
FlowScanner.MATCH_HAS_LOG);
if (logNode != null) {
String u = Jenkins.getActiveInstance().getRootUrl();
if (u == null) {
u = "http://jenkins/"; // placeholder
}
return u + logNode.getUrl() + logNode.getAction(LogAction.class).getUrlName();
}
}
return null;
Expand Down