[JENKINS-34064] Groovy 2.x fixes #24

jglick · 2016-04-06T21:39:37Z

Amends #23. Use with workflow-cps-plugin

diff --git a/pom.xml b/pom.xml
index 336f94c..88ac635 100644
--- a/pom.xml
+++ b/pom.xml
@@ -130,7 +130,7 @@
         <dependency>
             <groupId>com.cloudbees</groupId>
             <artifactId>groovy-cps</artifactId>
-            <version>1.7</version>
+            <version>1.8-SNAPSHOT</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.ui</groupId>

When running, say,

mvn -Dtest=org.jenkinsci.plugins.workflow.SerializationTest\#eachClosure -Djenkins.version=2.0-beta-2 test

this still fails (CpsDefaultGroovyMethods does not get used), but at least the build runs. Without this patch, basically everything (e.g., WorkflowTest#demo) breaks.

Have been trying to traverse the 2.x graph (as of groovy/groovy-core#395), so far without success:

diff --git a/src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java b/src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java
index 31fe372..d804943 100644
--- a/src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java
+++ b/src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java
@@ -20,6 +20,7 @@ import org.codehaus.groovy.runtime.metaclass.NewInstanceMetaMethod;
 import org.codehaus.groovy.util.AbstractConcurrentMapBase;
 import org.codehaus.groovy.util.AbstractConcurrentMapBase.Segment;
 import org.codehaus.groovy.util.FastArray;
+import org.codehaus.groovy.util.ManagedLinkedList;

 import java.lang.reflect.Field;
 import java.util.ArrayList;
@@ -92,6 +93,10 @@ class DGMPatcher {
     private final Field ClassInfo_globalClassSet = field(ClassInfo.class,"globalClassSet");
     private final Field AbstractConcurrentMapBase_segments = field(AbstractConcurrentMapBase.class,"segments");
     private final Field Segment_table = field(Segment.class,"table");
+    // Groovy 2
+    private final Field GlobalClassSet_items = field(ClassInfo.class.getName() + "$GlobalClassSet", "items");
+    private final Field ManagedLinkedList_head = field(ManagedLinkedList.class, "head");
+    private final Field ManagedLinkedList_Element_next = field(ManagedLinkedList.class.getName() + "$Element", "next");

     /**
      * Used to compare two {@link MetaMethod} by their signatures.
@@ -215,6 +220,15 @@ class DGMPatcher {
             patch(ci.getWeakMetaClass());
 //            patch(ci.getCachedClass());
         } else
+        if (o.getClass().getSimpleName().equals("GlobalClassSet")) {
+            patch(o, GlobalClassSet_items);
+        } else
+        if (o instanceof ManagedLinkedList) {
+            patch(o, ManagedLinkedList_head);
+        } else
+        if (o.getClass().getName().equals(ManagedLinkedList.class.getName() + "$Element")) {
+            patch(o, ManagedLinkedList_Element_next);
+        } else
 // doesn't look like we need to visit this
 //        if (o instanceof CachedClass) {
 //            CachedClass cc = (CachedClass) o;

@reviewbybees esp. @kohsuke

jglick · 2016-04-06T22:05:27Z

And of course if you update the groovy dependency in this POM to 2.4.6, you will get the expected test failure in eachArray. (Also each, though I am confused since this existed and presumably passed before #23, yet it seems to be testing essentially the same code path…?)

so returning the same type makes the patching code above easier

... that we need to rewrite differently.

While running tests, I came across a strange test failure where each(List,Closure) is delegating to each(Iterator,Closure) and results in a cast failure. Obviously, it shuold have resolved to each(Object,Closure) instead. This got me thinking about how Groovy resolves overloaded methods, which is non-trivial as a dynamic language. So in this change I'm changing the code so that Groovy doesn't have to resolve overloaded versions correctly.

The correct signature, which you can check in DefaultGroovyMethods, is to return the 'self' type

kohsuke · 2016-05-20T21:31:26Z

With these additional changes the serialization test in question now passes for me.

jglick · 2016-05-20T21:32:56Z

Awesome. Next week I will try to pick this up in an integration test.

kohsuke · 2016-05-20T21:36:43Z

To make the SerializationTest pass, I also needed jenkinsci/script-security-plugin#71 and jenkinsci/workflow-cps-plugin@a41aa9b

jglick · 2016-05-23T19:53:20Z

src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java

@@ -285,6 +308,10 @@ private Object patch(Object o) {
        return o;
    }

+    private boolean isInstance(Class t, Object o) {


Clearer to inline this I think.

…if only once, rather than merely returning the list element.

jglick · 2016-05-31T19:23:47Z

The fix does not seem to work, either in the unit test here, or in the downstream integration test. The closure is run only once.

jglick · 2016-05-31T19:30:22Z

Hmm, after

diff --git a/pom.xml b/pom.xml
index f511471..3731571 100644
--- a/pom.xml
+++ b/pom.xml
@@ -81,7 +81,7 @@
     <dependency>
       <groupId>org.codehaus.groovy</groupId>
       <artifactId>groovy</artifactId>
-      <version>1.8.9</version>
+      <version>2.4.6</version>
       <!--
         let the user of this library select the right flavor of groovy,
         including groovy vs groovy-all

I tried

mvn clean test -Dtest=CpsTransformerTest

which succeeds, yet

mvn clean test

fails:

Running com.cloudbees.groovy.cps.CpsTransformerTest
Tests run: 56, Failures: 1, Errors: 0, Skipped: 2, Time elapsed: 4.763 sec <<< FAILURE!
eachArray(com.cloudbees.groovy.cps.CpsTransformerTest)  Time elapsed: 0.029 sec  <<< FAILURE!
Assertion failed: 

assert resultInCps==sh.evaluate(script)
       |          | |  |        |
       11         | |  16        
                  | |                       def x = 10;
                  | |                       [1, 2, 3].each { y -> x+=y; }
                  | |                       return x;
                  | |                   
                  | groovy.lang.GroovyShell@1199fe66
                  false

    at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:402)
    at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:650)
    at com.cloudbees.groovy.cps.AbstractGroovyCpsTest.evalCPS(AbstractGroovyCpsTest.groovy:50)
    at com.cloudbees.groovy.cps.AbstractGroovyCpsTest$evalCPS.callCurrent(Unknown Source)
    at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:52)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:154)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:166)
    at com.cloudbees.groovy.cps.CpsTransformerTest.eachArray(CpsTransformerTest.groovy:620)
    at …

jglick · 2016-05-31T19:38:22Z

After

mvn clean install

here (without the Groovy version patch), from jenkinsci/workflow-cps-plugin#14

mvn -Dtest=SerializationTest\#eachClosure clean test

fails, as does

mvn -Dtest=SerializationTest\#eachClosure clean test -Djenkins.version=2.0

with

java.lang.AssertionError: Started
[Pipeline] node
Running on master in /tmp/junit8330663951182389291/junit2517820064459358411/workspace/demo
[Pipeline] {
[Pipeline] writeFile
[Pipeline] semaphore
[Pipeline] readFile
[Pipeline] echo
a
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Finished: SUCCESS

    at org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep.waitForStart(SemaphoreStep.java:144)
    at org.jenkinsci.plugins.workflow.SerializationTest$9.evaluate(SerializationTest.java:221)
    at …

indicating again that the closure was run just once.

jglick · 2016-05-31T20:08:16Z

This makes the unit test pass:

diff --git a/pom.xml b/pom.xml
index f511471..ac21096 100644
--- a/pom.xml
+++ b/pom.xml
@@ -54,6 +54,14 @@
           </dependency>
         </dependencies>
       </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>2.19.1</version>
+        <configuration>
+          <reuseForks>false</reuseForks>
+        </configuration>
+      </plugin>
     </plugins>
     <extensions>
       <extension>

jglick · 2016-05-31T21:01:03Z

The difference in behavior comes down to whether DGMPatcher.patch is called once per test suite, or once overall. A logger in CpsDefaultGroovyMethods._each confirms that it is not consistently called during failing runs.

To see the failure it suffices to run

mvn test -Dtest=CpsTransformerTest,GreenThreadTest

jglick · 2016-05-31T21:07:54Z

Successful run has:

running each(List, Closure) on []
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(List, Closure) on [1, 2, 3]
running each(List, Closure) on [1, 2, 3]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

whereas a failing run has:

running each(Object, Closure) on [org.codehaus.groovy.ast.Parameter@142269f2[name:y type: java.lang.Object, hasDefaultValue: false]]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(Object, Closure) on [org.codehaus.groovy.ast.Parameter@2d96543c[name:y type: java.lang.Object, hasDefaultValue: false]]
running each(Object, Closure) on [org.codehaus.groovy.ast.Parameter@3a4e343[name:v type: java.lang.Object, hasDefaultValue: false]]
running each(Object, Closure) on [org.codehaus.groovy.ast.Parameter@2cab9998[name:n type: int, hasDefaultValue: false], org.codehaus.groovy.ast.Parameter@2f7a7219[name:str type: java.lang.String, hasDefaultValue: false]]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
running each(List, Closure) on [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

kohsuke · 2016-06-27T17:12:44Z

I've tried all three ways you mention here in trying to reproduce the failure you are seeing, but to no avail thus far.

kohsuke · 2016-06-27T17:22:45Z

The difference in behavior comes down to whether DGMPatcher.patch is called once per test suite, or once overall. A logger in CpsDefaultGroovyMethods._each confirms that it is not consistently called during failing runs.

DGMPatcher.patch() gets invoked only once through the static initializer of a class, never "called once per test suite." Presumably Jesse means the difference comes down to the state of Groovy runtime when DGMPatcher.patch() is called.

The suspected cause of the problem is that DGMPatcher is not touching everything it needs to touch, when Groovy runtime is already warm and cached lots of MetaMethod objects. This test, having a name that is alphabetically before CpsTransformerTest, runs before that and populates the cache, exposing the defect in DGMPatcher. Run the test like this: mvn clean test -Dtest=AaaMakeGroovyBusy,CpsTransformerTest

jglick · 2016-06-27T17:43:03Z

I've tried all three ways you mention here in trying to reproduce the failure you are seeing, but to no avail thus far.

Try with Java 8. Passes for me too in 7, but fails in 8.

jglick · 2016-06-27T17:45:08Z

…but passes on 8 with Groovy 2.4.7.

... and cast wider net to traverse a bigger graph

…4' into DGMPatcher-Groovy-2-JENKINS-34064 Conflicts: src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java

…4064

Reverting #38 in this branch.

abayer · 2017-03-29T18:40:56Z

Since we've moved to Groovy 2.4.7 in 2.7.x and later lines, is it time to revisit this?

jglick · 2017-03-29T18:52:37Z

You mean, assuming we drop 1.x support in workflow-cps. I am OK with that, but it would not help with this. The problem here is that a fix (or hack, really) was developed for the issue that works with Groovy 1, but last I checked no one has managed to develop an analogue that works with Groovy 2. If we could, we would have merged this long ago, with at worst a simple 1-2 runtime switch.

Anyway I am not convinced DGMPatcher is the right approach. It is extremely brittle. I suspect the hooks used by GroovyCategorySupport would offer a simpler fix.

abayer · 2017-03-29T19:00:08Z

Ok, I'm gonna try to get to understand this a bit.

abayer · 2017-03-29T19:16:32Z

So it looks like https://issues.apache.org/jira/browse/GROOVY-6263 is the reason we can't use GroovyCategorySupport currently? That's fixed in 2.5.0-alpha-1, which is in the process of a release vote as we speak. Obviously, we don't want to depend on an alpha/beta/rc of Groovy, but I can start doing testing against that alpha release in the interim. I'm assuming https://github.com/cloudbees/groovy-cps/blob/master/src/main/java/com/cloudbees/groovy/cps/impl/ContinuationGroup.java#L67-L92 isn't actually the right way to use a category, given that the comment says it would affect the entire JVM...

jglick · 2017-03-29T21:32:06Z

I have no clue what that comment refers to. FWIW I was not suggesting actually using GroovyCategorySupport—it depends on ThreadLocal so there is no way it would work in CPS-transformed code, which uses virtual threads—but rather to study which (possibly “internal”) calls it makes in its implementation, and try to use those to convince the Groovy method dispatch system to honor our overrides of DefaultGroovyMethods. If that is even possible.

abayer · 2017-03-29T22:05:28Z

fwiw, I've started testing with Groovy 2.5.0-alpha-1 and have hit errors in CpsTransformerTest - specifically, each, eachArray, and superClass. I'm investigating to determine what changes in Groovy caused these.

abayer · 2017-03-29T22:51:42Z

I tell a lie - the problem change actually happened in 2.4.. With the following code...

import org.codehaus.groovy.runtime.InvokerHelper

public class A {
    @Override
    public String toString() {
        return "base"
    }
}

public class B extends A {
    @Override
    public String toString() {
        return "x" + super.toString()
    }
}

MetaClass mc = InvokerHelper.getMetaClass(B.class);

assert 'super$2$toString' == mc.getMethodWithCaching(B.class, "toString", null, true).getName();

...it passes with 2.4.7 and fails with 2.4.8. That results in DefaultInvoker.superCall(Class,Object,String,Object[]) barfing out with a CpsCallableInvocation rather than the super call, as best as I can tell.

Gonna find the actual change that caused this now. =)

abayer · 2017-03-29T23:09:13Z

And apache/groovy@0a6789d is the culprit commit. Now the question is do we want to open a bug against Groovy for this change from 2.4.7->2.4.8, or do we want to find a way to work around it in DefaultInvoker.superCall?

abayer · 2017-03-29T23:18:37Z

And as I keep digging - CpsTransformerTest.each and .eachArray broke going from Groovy 2.4.8 to 2.4.9, so unsurprisingly it's a different issue there. Doing some experiments to find that problem now.

abayer · 2017-03-30T15:05:10Z

Found the problem commit in Groovy for .each - apache/groovy@b18062a

abayer · 2017-03-30T16:15:52Z

Got it! GlobalClassSet changed to a new collection behind the scenes. I've got fixes for that and for the superClass failure (though I don't love that fix - it has the same result as the previous behavior but it feels awkward) up at https://github.com/abayer/groovy-cps/tree/jenkins-34064-continued

jglick · 2017-03-30T22:06:56Z

Not quite sure what it is you are fixing here—do you think there are regressions in Groovy 2.4.8 that have not yet been found by users, or what?

abayer · 2017-04-04T22:42:14Z

There definitely is - https://issues.apache.org/jira/browse/GROOVY-8140 is the superClass failure, legit regression in Groovy with a PR up at apache/groovy#520 that I'm hoping will make it into 2.5.0-alpha-2. The each and eachArray failures aren't due to a bug in Groovy, just a change in GlobalClassSet internally to Groovy with an easy fix.

abayer · 2017-04-11T00:39:43Z

So fwiw, https://github.com/abayer/groovy-cps/tree/jenkins-34064-continued, building against the latest Groovy 2.5.0-SNAPSHOT, has CategorySupport working properly. Woot.

abayer · 2017-04-11T01:13:08Z

I just realized what you meant re GroovyCategorySupport, so now I need to figure out what all that would entail when not done thread-wide.

abayer · 2017-05-11T16:46:02Z

Closing in favor of #51

jglick · 2017-05-11T18:52:54Z

BTW @abayer you do not need to close a PR which you propose to subsume in another PR; if the new one is merged, GH will automatically treat this one as merged, too.

[JENKINS-34064] Better robustness against reflection problems.

ae43475

jglick added the work-in-progress label Apr 6, 2016

jglick mentioned this pull request Apr 6, 2016

[JENKINS-34064] Backing out JENKINS-26481 fix jenkinsci/workflow-cps-plugin#2

Merged

kohsuke added 5 commits May 19, 2016 13:49

Groovy 2.x added this overload

1c07ff4

patch() is supposed to almost act idempotently

640f013

so returning the same type makes the patching code above easier

Groovy 2.x changed the signature of ClassInfo.globalClassSet

e660e30

... that we need to rewrite differently.

The problem in the previous commit was due to the return type!

fc5b7be

The correct signature, which you can check in DefaultGroovyMethods, is to return the 'self' type

kohsuke removed the work-in-progress label May 20, 2016

kohsuke assigned jglick May 20, 2016

jglick reviewed May 23, 2016
View reviewed changes

jglick mentioned this pull request May 31, 2016

[JENKINS-34064] Making fix work in Jenkins 2 jenkinsci/workflow-cps-plugin#14

Closed

Making it clearer from a test failure that we are running the block, …

f248a0f

…if only once, rather than merely returning the list element.

jglick added the work-in-progress label May 31, 2016

jglick mentioned this pull request Jun 6, 2016

[JENKINS-26481] More clearly report problem passing closures to binary methods jenkinsci/workflow-cps-plugin#15

Merged

kohsuke added 5 commits June 27, 2016 12:21

Remember which objects we traversed

0e27ded

... and cast wider net to traverse a bigger graph

Making this trace probe permanent

6ccbdb3

Testing out Jesse's hypothesis that hashCode has changed

a8b8e51

Testing the hypothesis that hash code is changing

2dda9e2

Merge remote-tracking branch 'origin/DGMPatcher-Groovy-2-JENKINS-3406…

ba19abb

…4' into DGMPatcher-Groovy-2-JENKINS-34064 Conflicts: src/main/java/com/cloudbees/groovy/cps/impl/DGMPatcher.java

jglick mentioned this pull request Aug 8, 2016

[JENKINS-34064] DGMPatcher is broken in Groovy 2, so do not try to use it at all #38

Merged

jglick added 2 commits August 9, 2016 09:09

Merge branch 'master' (early part) into DGMPatcher-Groovy-2-JENKINS-3…

7fa8551

…4064

Merge branch 'master' into DGMPatcher-Groovy-2-JENKINS-34064

68bc735

Reverting #38 in this branch.

jglick removed their assignment Dec 1, 2016

This was referenced May 10, 2017

[JENKINS-34064] Groovy 2.x fixes #51

Closed

[JENKINS-26481] Patch DGM methods with closure parameters #52

Closed

abayer closed this May 11, 2017

jglick deleted the DGMPatcher-Groovy-2-JENKINS-34064 branch May 23, 2017 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-34064] Groovy 2.x fixes #24

[JENKINS-34064] Groovy 2.x fixes #24

jglick commented Apr 6, 2016

jglick commented Apr 6, 2016

kohsuke commented May 20, 2016

jglick commented May 20, 2016

kohsuke commented May 20, 2016

jglick May 23, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

kohsuke commented Jun 27, 2016

kohsuke commented Jun 27, 2016

jglick commented Jun 27, 2016

jglick commented Jun 27, 2016

abayer commented Mar 29, 2017

jglick commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

jglick commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 30, 2017

abayer commented Mar 30, 2017

jglick commented Mar 30, 2017

abayer commented Apr 4, 2017

abayer commented Apr 11, 2017

abayer commented Apr 11, 2017

abayer commented May 11, 2017

jglick commented May 11, 2017

[JENKINS-34064] Groovy 2.x fixes #24

[JENKINS-34064] Groovy 2.x fixes #24

Conversation

jglick commented Apr 6, 2016

jglick commented Apr 6, 2016

kohsuke commented May 20, 2016

jglick commented May 20, 2016

kohsuke commented May 20, 2016

jglick May 23, 2016

Choose a reason for hiding this comment

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

jglick commented May 31, 2016

kohsuke commented Jun 27, 2016

kohsuke commented Jun 27, 2016

jglick commented Jun 27, 2016

jglick commented Jun 27, 2016

abayer commented Mar 29, 2017

jglick commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

jglick commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 29, 2017

abayer commented Mar 30, 2017

abayer commented Mar 30, 2017

jglick commented Mar 30, 2017

abayer commented Apr 4, 2017

abayer commented Apr 11, 2017

abayer commented Apr 11, 2017

abayer commented May 11, 2017

jglick commented May 11, 2017