Merge branch 'master' into finish-JENKINS-46076

pom.xml

@@ -71,8 +71,8 @@
         <git-plugin.version>3.1.0</git-plugin.version>
         <workflow-support-plugin.version>3.3</workflow-support-plugin.version>
         <scm-api-plugin.version>2.2.6</scm-api-plugin.version>
-        <groovy-cps.version>1.27</groovy-cps.version>
-         <structs-plugin.version>1.18</structs-plugin.version>
+        <groovy-cps.version>1.28</groovy-cps.version>
+        <structs-plugin.version>1.18</structs-plugin.version>
         <workflow-step-api-plugin.version>2.20</workflow-step-api-plugin.version>
     </properties>
     <dependencies>

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsVmExecutorService.java

@@ -1,15 +1,17 @@
 package org.jenkinsci.plugins.workflow.cps;
 
+import com.cloudbees.groovy.cps.impl.CpsCallableInvocation;
+import hudson.Main;
 import hudson.model.Computer;
 import hudson.remoting.SingleLaneExecutorService;
 import hudson.security.ACL;
-import jenkins.util.InterceptingExecutorService;
-
+import java.io.IOException;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutorService;
+import java.util.logging.Level;
 import java.util.logging.Logger;
-
-import static java.util.logging.Level.WARNING;
+import javax.annotation.CheckForNull;
+import jenkins.util.InterceptingExecutorService;
 
 /**
  * {@link ExecutorService} for running CPS VM.
@@ -50,7 +52,7 @@ public void run() {
      * That makes it worth reporting.
      */
     private void reportProblem(Throwable t) {
-        LOGGER.log(WARNING, "Unexpected exception in CPS VM thread: " + cpsThreadGroup.getExecution(), t);
+        LOGGER.log(Level.WARNING, "Unexpected exception in CPS VM thread: " + cpsThreadGroup.getExecution(), t);
         cpsThreadGroup.getExecution().croak(t);
     }
 
@@ -105,9 +107,54 @@ private ThreadContext setUp() {
             assert cpsThreadGroup.getExecution().getShell().getClassLoader() != null;
             t.setContextClassLoader(cpsThreadGroup.getExecution().getShell().getClassLoader());
         }
+        CpsCallableInvocation.registerMismatchHandler(this::handleMismatch);
         return context;
     }
 
+    private void handleMismatch(Object expectedReceiver, String expectedMethodName, Object actualReceiver, String actualMethodName) {
+        String mismatchMessage = mismatchMessage(className(expectedReceiver), expectedMethodName, className(actualReceiver), actualMethodName);
+        if (FAIL_ON_MISMATCH) {
+            throw new IllegalStateException(mismatchMessage);
+        } else {
+            try {
+                cpsThreadGroup.getExecution().getOwner().getListener().getLogger().println(mismatchMessage);
+            } catch (IOException x) {
+                LOGGER.log(Level.FINE, null, x);
+            }
+        }
+    }
+
+    private static @CheckForNull String className(@CheckForNull Object receiver) {
+        if (receiver == null) {
+            return null;
+        } else if (receiver instanceof Class) {
+            return ((Class) receiver).getName();
+        } else {
+            return receiver.getClass().getName();
+        }
+    }
+
+    /**
+     * Making false positives be fatal makes it much easier to detect mistakes here and in PCT.
+     * But we would rather have this be nonfatal in production,
+     * since there are sure to be some false positives in exotic situations not yet covered by tests.
+     * (As well as some false negatives, but this is a best effort after all.)
+     */
+    static boolean FAIL_ON_MISMATCH = Main.isUnitTest;
+
+    static String mismatchMessage(@CheckForNull String expectedReceiverClassName, String expectedMethodName, @CheckForNull String actualReceiverClassName, String actualMethodName) {
+        StringBuilder b = new StringBuilder("expected to call ");
+        if (expectedReceiverClassName != null) {
+            b.append(expectedReceiverClassName).append('.');
+        }
+        b.append(expectedMethodName).append(" but wound up catching ");
+        if (actualReceiverClassName != null) {
+            b.append(actualReceiverClassName).append('.');
+        }
+        b.append(actualMethodName);
+        return b.append("; see: https://jenkins.io/redirect/pipeline-cps-method-mismatches/").toString();
+    }
+
     private void tearDown(ThreadContext context) {
         CURRENT.set(null);
         cpsThreadGroup.busy = false;
@@ -116,6 +163,7 @@ private void tearDown(ThreadContext context) {
         if (isShutdown()) {
             execution.logTimings();
         }
+        CpsCallableInvocation.registerMismatchHandler(null);
     }
 
     static ThreadLocal<CpsThreadGroup> CURRENT = new ThreadLocal<>();

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

@@ -158,6 +158,7 @@ public Object invokeMethod(String name, Object args) {
             }
             unreportedAmbiguousFunctions = new HashSet<>(0);
             for (StepDescriptor d : StepDescriptor.all()) {
+                // TODO consider adding metasteps here and in reportAmbiguousStepInvocation
                 String functionName = d.getFunctionName();
                 if (functions.containsKey(functionName)) {
                     unreportedAmbiguousFunctions.add(functionName);
@@ -210,7 +211,7 @@ protected Object invokeStep(StepDescriptor d, Object args) {
     /**
      * When {@link #invokeMethod(String, Object)} is calling a {@link StepDescriptor}
      * @param d The {@link StepDescriptor} being invoked.
-     * @param name The name used to invoke the step. Will be either {@code d.getFunctionName()} or {@code d.clazz.getName()}.
+     * @param name The name used to invoke the step. May be {@link StepDescriptor#getFunctionName}, a symbol as in {@link StepDescriptor#metaStepsOf}, or {@code d.clazz.getName()}.
      * @param args The arguments passed to the step.
      */
     protected Object invokeStep(StepDescriptor d, String name, Object args) {
@@ -296,7 +297,7 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) {
         } else {
             // if it's in progress, suspend it until we get invoked later.
             // when it resumes, the CPS caller behaves as if this method returned with the resume value
-            Continuable.suspend(new ThreadTaskImpl(context));
+            Continuable.suspend(name, new ThreadTaskImpl(context));
 
             // the above method throws an exception to unwind the call stack, and
             // the control then goes back to CpsFlowExecution.runNextChunk
@@ -392,7 +393,7 @@ protected Object invokeDescribable(String symbol, Object _args) {
                 ud = new UninstantiatedDescribable(symbol, null, dargs);
                 margs.put(p.getName(),ud);
 
-                return invokeStep(metaStep,new NamedArgsAndClosure(margs,args.body));
+                return invokeStep(metaStep, symbol, new NamedArgsAndClosure(margs, args.body));
             } catch (Exception e) {
                 throw new IllegalArgumentException("Failed to prepare "+symbol+" step",e);
             }

src/main/java/org/jenkinsci/plugins/workflow/cps/Safepoint.java

@@ -21,7 +21,7 @@ public class Safepoint extends ThreadTask {
      */
     @Whitelisted
     public static void safepoint() {
-        Continuable.suspend(new Safepoint());
+        Continuable.suspend("safepoint", new Safepoint());
     }
 
     @Override

src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java

@@ -16,7 +16,6 @@
 import org.jvnet.hudson.test.TestExtension;
 import org.kohsuke.stapler.DataBoundConstructor;
 import static org.junit.Assert.assertTrue;
-import org.junit.Ignore;
 import org.jvnet.hudson.test.Issue;
 
 /**
@@ -224,23 +223,4 @@ public boolean start() throws Exception {
         });
     }
 
-    @Ignore("TODO JENKINS-31314: calls writeFile just once, echoes null (i.e., return value of writeFile), then succeeds")
-    @Test public void nonCpsContinuable() {
-        story.addStep(new Statement() {
-            @Override public void evaluate() throws Throwable {
-                p = jenkins().createProject(WorkflowJob.class, "demo");
-                p.setDefinition(new CpsFlowDefinition(
-                    "@NonCPS def shouldBomb() {\n" +
-                    "  def text = ''\n" +
-                    "  ['a', 'b', 'c'].each {it -> writeFile file: it, text: it; text += it}\n" +
-                    "  text\n" +
-                    "}\n" +
-                    "node {\n" +
-                    "  echo shouldBomb()\n" +
-                    "}\n", true));
-                b = story.j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
-            }
-        });
-    }
-
 }

src/test/java/org/jenkinsci/plugins/workflow/cps/CpsVmExecutorServiceTest.java

@@ -24,22 +24,103 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
+import hudson.model.Result;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.Rule;
+import org.junit.rules.ErrorCollector;
 import org.jvnet.hudson.test.BuildWatcher;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 public class CpsVmExecutorServiceTest {
 
     @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public JenkinsRule r = new JenkinsRule();
+    @Rule public ErrorCollector errors = new ErrorCollector();
 
     @Test public void contextClassLoader() throws Exception {
         WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
         p.setDefinition(new CpsFlowDefinition("echo(/yes I can load ${Thread.currentThread().contextClassLoader.loadClass(getClass().name)}/)", false));
         r.buildAndAssertSuccess(p);
     }
 
+    @Issue({"JENKINS-31314", "JENKINS-27306", "JENKINS-26313"})
+    @Test public void wrongCatcher() throws Exception {
+        boolean origFailOnMismatch = CpsVmExecutorService.FAIL_ON_MISMATCH;
+        CpsVmExecutorService.FAIL_ON_MISMATCH = false;
+        try {
+            WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("def ok() {sleep 1}; @NonCPS def bad() {for (int i = 0; i < 10; i++) {sleep 1}; assert false : 'never gets here'}; node {ok(); bad()}", true));
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("WorkflowScript", "bad", null, "sleep"), r.buildAndAssertSuccess(p));
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("def l = [3, 2, 1]; println(/oops got ${l.sort {x, y -> x - y}}/)", true));
+                WorkflowRun b = r.buildAndAssertSuccess(p);
+                r.assertLogContains("oops got -1", b);
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("java.util.ArrayList", "sort", "org.jenkinsci.plugins.workflow.cps.CpsClosure2", "call"), b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("node {[1, 2, 3].each {x -> sleep 1; echo(/no problem got $x/)}}", true));
+                WorkflowRun b = r.buildAndAssertSuccess(p);
+                r.assertLogContains("no problem got 3", b);
+                r.assertLogNotContains("expected to call", b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("class C {@Override String toString() {'never used'}}; def gstring = /embedding ${new C()}/; echo(/oops got $gstring/)", true));
+                WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); // JENKINS-27306: No such constructor found: new org.codehaus.groovy.runtime.GStringImpl java.lang.String java.lang.String[]
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("org.codehaus.groovy.runtime.ScriptBytecodeAdapter", "asType", "C", "toString"), b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("echo(/see what ${-> 'this'} does/)", true));
+                WorkflowRun b = r.buildAndAssertSuccess(p);
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("WorkflowScript", "echo", "org.jenkinsci.plugins.workflow.cps.CpsClosure2", "call"), b);
+                r.assertLogNotContains("see what", b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("node {echo(/see what ${-> 'this'} does/)}", true));
+                WorkflowRun b = r.buildAndAssertSuccess(p);
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("WorkflowScript", "echo", "org.jenkinsci.plugins.workflow.cps.CpsClosure2", "call"), b);
+                r.assertLogNotContains("see what", b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition(
+                    "@NonCPS def shouldBomb() {\n" +
+                    "  def text = ''\n" +
+                    "  ['a', 'b', 'c'].each {it -> writeFile file: it, text: it; text += it}\n" +
+                    "  text\n" +
+                    "}\n" +
+                    "node {\n" +
+                    "  echo shouldBomb()\n" +
+                    "}\n", true));
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("WorkflowScript", "shouldBomb", null, "writeFile"), r.buildAndAssertSuccess(p));
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("@NonCPS def bad() {polygon(17) {}}; bad()", true));
+                WorkflowRun b = r.buildAndAssertSuccess(p);
+                r.assertLogContains("wrapping in a 17-gon", b);
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("WorkflowScript", "bad", null, "polygon"), b);
+                return null;
+            });
+            errors.checkSucceeds(() -> {
+                p.setDefinition(new CpsFlowDefinition("class C {C(script) {script.sleep(1)}}; new C(this)", true));
+                WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0));
+                r.assertLogContains(CpsVmExecutorService.mismatchMessage("C", "<init>", null, "sleep"), b);
+                return null;
+            });
+        } finally {
+            CpsVmExecutorService.FAIL_ON_MISMATCH = origFailOnMismatch;
+        }
+    }
+
 }

src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java

@@ -134,13 +134,10 @@ public void pauseInsideLoad() throws Exception {
                 ScriptApproval.get().approveSignature("method groovy.lang.Binding getVariables");
                 jenkins.getWorkspaceFor(p).child("b.groovy").write("def m(arg) {echo \"${this} binding=${binding.variables}\"; a(\"${arg} from b\")}; this", null);
                 // Control case:
-                // TODO if you enable sandbox here, build fails: NoSuchMethodError: No such DSL method 'a' found among […]
-                // SandboxInterceptor.onMethodCall is given Script2 as the receiver and "a" as the method, when really it should be asked about onGetProperty(Script2.a) followed by onMethodCall(Script1.call).
-                // Works fine if you use a.call(…) rather than a(…).
-                p.setDefinition(new CpsFlowDefinition("a = 0; node {a = load 'a.groovy'}; def b; node {b = load 'b.groovy'}; echo \"${this} binding=${binding.variables}\"; b.m('value')", false));
+                p.setDefinition(new CpsFlowDefinition("a = 0; node {a = load 'a.groovy'}; def b; node {b = load 'b.groovy'}; echo \"${this} binding=${binding.variables}\"; b.m('value')", true));
                 story.j.assertLogContains("a ran on value from b", story.j.assertBuildStatusSuccess(p.scheduleBuild2(0)));
                 // Test case:
-                p.setDefinition(new CpsFlowDefinition("a = 0; node {a = load 'a.groovy'}; semaphore 'wait'; def b; node {b = load 'b.groovy'}; echo \"${this} binding=${binding.variables}\"; b.m('value')", /* TODO ditto */false));
+                p.setDefinition(new CpsFlowDefinition("a = 0; node {a = load 'a.groovy'}; semaphore 'wait'; def b; node {b = load 'b.groovy'}; echo \"${this} binding=${binding.variables}\"; b.m('value')", true));
                 WorkflowRun b = p.scheduleBuild2(0).getStartCondition().get();
                 SemaphoreStep.waitForStart("wait/1", b);
             }