Merge pull request #1061 from dwnusbaum/failureadapter-npe

Prevent `NullPointerException` in `CpsBodyExecution.FailureAdapter`

Author: jglick

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java

@@ -375,9 +375,14 @@ private Throwable handleError(CpsBodySubContext sc, Throwable t) {
                 localThread = thread;
             }
             try {
-                var handler = localThread.getContextVariables().get(FailureHandler.class, localThread::getExecution, sc::getNode);
-                if (handler != null) {
-                    return handler.handle(sc, t);
+                var contextVars = localThread.getContextVariables();
+                // CpsFlowExecution.start sets the context variables for the initial CpsThread to null, so context
+                // variables for all subsequent threads remain null until a block step adds a variable to its body.
+                if (contextVars != null) {
+                    var handler = contextVars.get(FailureHandler.class, localThread::getExecution, sc::getNode);
+                    if (handler != null) {
+                        return handler.handle(sc, t);
+                    }
                 }
             } catch (Throwable t2) {
                 t.addSuppressed(t2);

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodySubContext.java

@@ -35,7 +35,7 @@ final class CpsBodySubContext extends DefaultStepContext {
 
     @NonNull
     @Override
-    protected FlowNode getNode() throws IOException {
+    protected FlowNode getNode() {
         return node;
     }
 

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java

@@ -144,6 +144,7 @@ <T> T getContextVariable(Class<T> key, ContextVariableSet.ThrowingSupplier<FlowE
         }
     }
 
+    @CheckForNull
     public ContextVariableSet getContextVariables() {
         return contextVariables;
     }
@@ -236,6 +237,7 @@ boolean isAlive() {
      * something is unexpectedly holding a reference directly to it (see JENKINS-63164 for an example scenario).
      */
     void cleanUp() {
+        LOGGER.finest(() -> "Cleaning up " + this);
         program = null;
         resumeValue = null;
         step = null;

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecutionTest.java

@@ -26,7 +26,11 @@
 import org.jenkinsci.plugins.workflow.steps.AbstractStepImpl;
 import org.jenkinsci.plugins.workflow.steps.BodyExecution;
 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.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import org.junit.ClassRule;
 import org.junit.Rule;
@@ -273,4 +277,35 @@ public static class Execution extends AbstractStepExecutionImpl {
         });
     }
 
+    @Test public void failureWithNoContextVars() throws Throwable {
+        rr.then(r -> {
+            WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition("noOpBlock { throw new Exception('oops') }", true));
+            var b = r.buildAndAssertStatus(Result.FAILURE, p);
+            r.assertLogNotContains("java.lang.NullPointerException: Cannot invoke \"org.jenkinsci.plugins.workflow.cps.ContextVariableSet.get", b);
+        });
+    }
+
+    public static class NoOpBlockStep extends Step {
+        @DataBoundConstructor
+        public NoOpBlockStep() {}
+        @Override
+        public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.block(context, (ctx, inv) -> {});
+        }
+        @TestExtension("failureWithNoContextVars") public static class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "noOpBlock";
+            }
+            @Override
+            public boolean takesImplicitBlockArgument() {
+                return true;
+            }
+            @Override
+            public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of();
+            }
+        }
+    }
+
 }