diff --git a/pom.xml b/pom.xml index 3c420334..da483158 100644 --- a/pom.xml +++ b/pom.xml @@ -73,7 +73,7 @@ io.jenkins.tools.bom bom-2.426.x - 2555.v3190a_8a_c60c6 + 2745.vc7b_fe4c876fa_ import pom diff --git a/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java b/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java index a6853bff..f8d58295 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java @@ -37,6 +37,8 @@ import java.util.Objects; import java.util.Set; import java.util.UUID; +import java.util.logging.Level; +import java.util.logging.Logger; import jenkins.model.Jenkins; import org.apache.commons.io.output.NullOutputStream; import org.jenkinsci.plugins.workflow.flow.FlowExecution; @@ -53,23 +55,33 @@ */ public class ErrorAction implements PersistentAction { + private static final Logger LOGGER = Logger.getLogger(ErrorAction.class.getName()); + private final @NonNull Throwable error; public ErrorAction(@NonNull Throwable error) { + Throwable errorForAction = error; if (isUnserializableException(error, new HashSet<>())) { - error = new ProxyException(error); + LOGGER.log(Level.FINE, "sanitizing unserializable error", error); + errorForAction = new ProxyException(error); } else if (error != null) { try { Jenkins.XSTREAM2.toXMLUTF8(error, new NullOutputStream()); } catch (Exception x) { + LOGGER.log(Level.FINE, "unable to serialize to XML", x); // Typically SecurityException from ClassFilter. - error = new ProxyException(error); + errorForAction = new ProxyException(error); } } - this.error = error; + this.error = errorForAction; String id = findId(error, new HashSet<>()); - if (id == null && error != null) { - error.addSuppressed(new ErrorId()); + if (id == null) { + var errorId = new ErrorId(); + errorForAction.addSuppressed(errorId); + if (error != errorForAction) { + // Make sure the original exception has the error ID, not just the copy here. + error.addSuppressed(errorId); + } } } @@ -170,28 +182,49 @@ public String getUrlName() { @Restricted(Beta.class) public static boolean equals(Throwable t1, Throwable t2) { if (t1 == t2) { + LOGGER.fine(() -> "Same object: " + t1); return true; - } else if (t1.getClass() != t2.getClass()) { + } + boolean noProxy = t1.getClass() != ProxyException.class && t2.getClass() != ProxyException.class; + if (noProxy && t1.getClass() != t2.getClass()) { + LOGGER.fine(() -> "Different types: " + t1.getClass() + " vs. " + t2.getClass()); return false; - } else if (!Objects.equals(t1.getMessage(), t2.getMessage())) { + } else if (noProxy && !Objects.equals(t1.getMessage(), t2.getMessage())) { + LOGGER.fine(() -> "Different messages: " + t1.getMessage() + " vs. " + t2.getMessage()); return false; } else { String id1 = findId(t1, new HashSet<>()); if (id1 != null) { - return id1.equals(findId(t2, new HashSet<>())); + String id2 = findId(t2, new HashSet<>()); + if (id1.equals(id2)) { + LOGGER.fine(() -> "ErrorId matches: " + id1); + return true; + } else { + LOGGER.fine(() -> "ErrorId mismatch: " + t1 + " " + id1 + " vs. " + t2 + " " + id2); + return false; + } } // No ErrorId, use a best-effort approach that doesn't work across restarts for exceptions thrown // synchronously from the CPS VM thread. // Check that stack traces match, but specifically avoid checking suppressed exceptions, which are often // modified after ErrorAction is written to disk when steps like parallel are involved. - while (t1 != null && t2 != null) { - if (!Arrays.equals(t1.getStackTrace(), t2.getStackTrace())) { + var _t1 = t1; + var _t2 = t2; + while (_t1 != null && _t2 != null) { + if (!Arrays.equals(_t1.getStackTrace(), _t2.getStackTrace())) { + LOGGER.fine(() -> "Different stack traces between " + t1 + " vs. " + t2); // not showing details return false; } - t1 = t1.getCause(); - t2 = t2.getCause(); + _t1 = _t1.getCause(); + _t2 = _t2.getCause(); + } + if ((_t1 == null) == (_t2 == null)) { + LOGGER.fine(() -> "Same stack traces in " + t1 + " vs. " + t2); + return true; + } else { + LOGGER.fine(() -> "Different cause depths between " + t1 + " vs. " + t2); + return false; } - return (t1 == null) == (t2 == null); } } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java b/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java index 6165330e..6ec2ac99 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java @@ -47,21 +47,44 @@ import org.jvnet.hudson.test.JenkinsSessionRule; import groovy.lang.MissingMethodException; +import hudson.FilePath; import hudson.Functions; import hudson.model.Result; +import hudson.model.TaskListener; import hudson.remoting.ProxyException; +import hudson.remoting.VirtualChannel; +import java.io.File; +import java.io.IOException; +import java.util.Set; +import java.util.logging.Level; +import jenkins.MasterToSlaveFileCallable; import org.codehaus.groovy.runtime.NullObject; +import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback; +import org.jenkinsci.plugins.workflow.steps.Step; +import org.jenkinsci.plugins.workflow.steps.StepContext; +import org.jenkinsci.plugins.workflow.steps.StepDescriptor; +import org.jenkinsci.plugins.workflow.steps.StepExecution; +import org.jenkinsci.plugins.workflow.steps.StepExecutions; +import org.jvnet.hudson.test.InboundAgentRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.DataBoundConstructor; /** * Tests for {@link ErrorAction} */ public class ErrorActionTest { + @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); @Rule public JenkinsSessionRule rr = new JenkinsSessionRule(); + @Rule public InboundAgentRule agents = new InboundAgentRule(); + + @Rule public LoggerRule logging = new LoggerRule().record(ErrorAction.class, Level.FINE); + private List extractErrorActions(FlowExecution exec) { List ret = new ArrayList<>(); @@ -228,6 +251,87 @@ public static class X extends Exception { }); } + @Test public void findOriginFromBodyExecutionCallback() throws Throwable { + rr.then(r -> { + agents.createAgent(r, "remote"); + var p = r.createProject(WorkflowJob.class); + p.setDefinition(new CpsFlowDefinition("callsFindOrigin {node('remote') {fails()}}", true)); + var b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("Acting slowly in ", b); + agents.stop("remote"); + r.assertBuildStatus(Result.FAILURE, r.waitForCompletion(b)); + r.assertLogContains("Found in: fails", b); + }); + } + public static final class WrapperStep extends Step { + @DataBoundConstructor public WrapperStep() {} + @Override public StepExecution start(StepContext context) throws Exception { + return new ExecutionImpl(context); + } + private static final class ExecutionImpl extends StepExecution { + ExecutionImpl(StepContext context) { + super(context); + } + @Override public boolean start() throws Exception { + getContext().newBodyInvoker().withCallback(new Callback()).start(); + return false; + } + } + private static class Callback extends BodyExecutionCallback { + @Override public void onSuccess(StepContext context, Object result) { + context.onSuccess(result); + } + @Override + public void onFailure(StepContext context, Throwable t) { + try { + var l = context.get(TaskListener.class); + Functions.printStackTrace(t, l.error("Original failure:")); + l.getLogger().println("Found in: " + ErrorAction.findOrigin(t, context.get(FlowExecution.class)).getDisplayFunctionName()); + } catch (Exception x) { + assert false : x; + } + context.onFailure(t); + } + } + @TestExtension("findOriginFromBodyExecutionCallback") public static final class DescriptorImpl extends StepDescriptor { + @Override public String getFunctionName() { + return "callsFindOrigin"; + } + @Override public Set> getRequiredContext() { + return Set.of(); + } + @Override public boolean takesImplicitBlockArgument() { + return true; + } + } + } + public static final class FailingStep extends Step { + @DataBoundConstructor public FailingStep() {} + @Override public StepExecution start(StepContext context) throws Exception { + return StepExecutions.synchronousNonBlockingVoid(context, c -> c.get(FilePath.class).act(new Sleep(c.get(TaskListener.class)))); + } + private static final class Sleep extends MasterToSlaveFileCallable { + private final TaskListener l; + Sleep(TaskListener l) { + this.l = l; + } + @Override public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { + l.getLogger().println("Acting slowly in " + f); + l.getLogger().flush(); + Thread.sleep(Long.MAX_VALUE); + return null; + } + } + @TestExtension("findOriginFromBodyExecutionCallback") public static final class DescriptorImpl extends StepDescriptor { + @Override public String getFunctionName() { + return "fails"; + } + @Override public Set> getRequiredContext() { + return Set.of(FilePath.class); + } + } + } + @Test public void cyclicErrorsAreSupported() throws Throwable { Exception cyclic1 = new Exception(); Exception cyclic2 = new Exception(cyclic1);