[JENKINS-26481] More clearly report problem passing closures to binary methods #15

jglick · 2016-06-06T14:05:47Z

Tries to improve diagnostics until cloudbees/groovy-cps#24 can be fixed.

Before this patch, SerializationTest.eachClosure fails with cryptic script behavior:

Started
[Pipeline] node
Running on master in /tmp/junit3096297553890442745/junit8071484631883149575/workspace/demo
[Pipeline] {
[Pipeline] writeFile
[Pipeline] semaphore
[Pipeline] readFile
[Pipeline] echo
a
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Finished: SUCCESS

Note that there is no error in the build, it just does not run correctly. After the patch, the build fails:

Started
[Pipeline] node
Running on master in /tmp/junit8559386618399120809/junit4301051117364074727/workspace/demo
[Pipeline] {
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
java.lang.UnsupportedOperationException: Calling public static java.lang.Object org.codehaus.groovy.runtime.DefaultGroovyMethods.each(java.lang.Object,groovy.lang.Closure) on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops
    at org.jenkinsci.plugins.workflow.cps.GroovyClassLoaderWhitelist.checkJenkins26481(GroovyClassLoaderWhitelist.java:84)
    at org.jenkinsci.plugins.workflow.cps.GroovyClassLoaderWhitelist.permitsStaticMethod(GroovyClassLoaderWhitelist.java:54)
    at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onMethodCall(SandboxInterceptor.java:83)
    at org.kohsuke.groovy.sandbox.impl.Checker$1.call(Checker.java:149)
    at org.kohsuke.groovy.sandbox.impl.Checker.checkedCall(Checker.java:146)
    at com.cloudbees.groovy.cps.sandbox.SandboxInvoker.methodCall(SandboxInvoker.java:15)
    at WorkflowScript.run(WorkflowScript:2)
    at ___cps.transform___(Native Method)
    at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:55)
    at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.dispatchOrArg(FunctionCallBlock.java:106)
    at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.fixArg(FunctionCallBlock.java:79)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
    at com.cloudbees.groovy.cps.impl.ClosureBlock.eval(ClosureBlock.java:40)
    at com.cloudbees.groovy.cps.Next.step(Next.java:58)
    at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:154)
    at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.access$001(SandboxContinuable.java:18)
    at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:32)
    at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:29)
    at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.runInSandbox(GroovySandbox.java:106)
    at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.run0(SandboxContinuable.java:29)
    at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:164)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:276)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$000(CpsThreadGroup.java:78)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:185)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:183)
    at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:47)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
    at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Finished: FAILURE

@reviewbybees esp. @kohsuke

… a CPS-transformed closure to a binary method.

…ally requested a Closure, and is thus likely to try to call it.

jglick · 2016-06-06T14:08:41Z

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

+            }
+        });
+    }
+
    @Ignore("TODO JENKINS-31314: calls writeFile just once, echoes null (i.e., return value of writeFile), then succeeds")


Still fails with the same behavior.

abayer · 2016-06-09T15:55:31Z

🐝 Happy to see this!

oleg-nenashev · 2016-06-09T16:06:32Z

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

    private final ClassLoader scriptLoader;

-    public GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader) {


We do not expect other plugins to have classes in the org.jenkinsci.plugins.workflow.cps package, right?

oleg-nenashev · 2016-06-09T16:08:42Z

🐝 if there is no compatibility risk

oleg-nenashev · 2016-06-09T16:14:49Z

re- 🐝

abayer · 2016-06-09T16:29:55Z

re 🐝 as well.

jglick · 2016-06-09T17:11:14Z

Will wait to see if @kohsuke has any concerns about this. Obviously it passes local tests, but there is some risk of inadvertently failing legitimate, previously working calls.

jglick · 2016-06-09T20:16:05Z

@kohsuke asks for confirmation that this does not break usages of closures in global libraries, and I guess I would add load. I think it should be fine, as these use GroovyClassLoader, but it is worth verifying in a test.

…s defined in load’d scripts.

jglick · 2016-06-09T20:34:22Z

And I checked that workflow-cps-global-lib tests pass against this PR, including WorkflowLibRepositoryTest.userDefinedGlobalVariable which passes a CPS-transformed closure to a global variable. If I patch it as

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/global/WorkflowLibRepositoryTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/global/WorkflowLibRepositoryTest.java
index b814207..b1845b8 100644
--- a/src/test/java/org/jenkinsci/plugins/workflow/cps/global/WorkflowLibRepositoryTest.java
+++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/global/WorkflowLibRepositoryTest.java
@@ -116,6 +116,7 @@ public class WorkflowLibRepositoryTest {
                         "  body.resolveStrategy = Closure.DELEGATE_FIRST",
                         "  body.delegate = config",
                         "  body()",
+                        "  [config.title].each {x -> echo x}",
                         "  echo 'title was '+config.title",
                         "}")
                         , "\n"));

then it fails with the expected UnsupportedOperationException.

abayer · 2016-06-09T22:01:36Z

re-re-:bee:!

…ovyMethods.

… but test does pass now.

jglick added 2 commits June 6, 2016 09:49

[JENKINS-26481] Fail the script meaningfully when it attempts to pass…

efeee2b

… a CPS-transformed closure to a binary method.

[JENKINS-26481] Limit the error to cases where the binary method actu…

6ad4d04

…ally requested a Closure, and is thus likely to try to call it.

jglick reviewed Jun 6, 2016
View reviewed changes

java.lang.reflect.Executable does not exist in Java 7.

0d2af49

oleg-nenashev reviewed Jun 9, 2016
View reviewed changes

jglick added 2 commits June 9, 2016 12:11

Merge branch 'master' into report-JENKINS-26481

e79482a

@oleg-nenashev requested that a PR comment be moved into Javadoc.

ae55312

Merge branch 'master' into report-JENKINS-26481

62a8792

[JENKINS-26481] Verifying that we can still pass closures to function…

4266f0c

…s defined in load’d scripts.

jglick merged commit 0043928 into jenkinsci:master Jun 10, 2016

jglick deleted the report-JENKINS-26481 branch June 10, 2016 12:53

jglick added a commit that referenced this pull request Jun 10, 2016

Merged with #15.

2f70a8d

jglick added a commit that referenced this pull request Jun 10, 2016

#15 needs to be suppressed if we make it actually work for DefaultGro…

4c898ca

…ovyMethods.

This was referenced May 11, 2017

[JENKINS-34064] Groovy 2.x fixes cloudbees/groovy-cps#51

Closed

[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

Merged

dwnusbaum pushed a commit to dwnusbaum/workflow-cps-plugin that referenced this pull request Jun 1, 2020

Merging jenkinsci#30. Fixes jenkinsci#15. Not sure when it was fixed,…

837eabc

… but test does pass now.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-26481] More clearly report problem passing closures to binary methods #15

[JENKINS-26481] More clearly report problem passing closures to binary methods #15

jglick commented Jun 6, 2016

jglick Jun 6, 2016

abayer commented Jun 9, 2016

oleg-nenashev Jun 9, 2016

jglick Jun 9, 2016

oleg-nenashev commented Jun 9, 2016

oleg-nenashev commented Jun 9, 2016

abayer commented Jun 9, 2016

jglick commented Jun 9, 2016

jglick commented Jun 9, 2016

jglick commented Jun 9, 2016

abayer commented Jun 9, 2016

		private final ClassLoader scriptLoader;

		public GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader) {

[JENKINS-26481] More clearly report problem passing closures to binary methods #15

[JENKINS-26481] More clearly report problem passing closures to binary methods #15

Conversation

jglick commented Jun 6, 2016

jglick Jun 6, 2016

Choose a reason for hiding this comment

abayer commented Jun 9, 2016

oleg-nenashev Jun 9, 2016

Choose a reason for hiding this comment

jglick Jun 9, 2016

Choose a reason for hiding this comment

oleg-nenashev commented Jun 9, 2016

oleg-nenashev commented Jun 9, 2016

abayer commented Jun 9, 2016

jglick commented Jun 9, 2016

jglick commented Jun 9, 2016

jglick commented Jun 9, 2016

abayer commented Jun 9, 2016