Merge pull request #328 from jglick/findOriginFromBodyExecutionCallback

`ErrorAction.findOrigin` failures due to `ProxyException`

pom.xml

@@ -73,7 +73,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.426.x</artifactId>
-                <version>2555.v3190a_8a_c60c6</version>
+                <version>2745.vc7b_fe4c876fa_</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>

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);
         }
     }
 

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<ErrorAction> extractErrorActions(FlowExecution exec) {
         List<ErrorAction> 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<? extends Class<?>> 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<Void> {
+            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<? extends Class<?>> getRequiredContext() {
+                return Set.of(FilePath.class);
+            }
+        }
+    }
+
     @Test public void cyclicErrorsAreSupported() throws Throwable {
         Exception cyclic1 = new Exception();
         Exception cyclic2 = new Exception(cyclic1);