Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
May 30, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 41 additions & 2 deletions src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@

package org.jenkinsci.plugins.workflow.cps;

import com.cloudbees.groovy.cps.Builder;
import com.cloudbees.groovy.cps.Continuable;
import com.cloudbees.groovy.cps.CpsDefaultGroovyMethods;
import com.cloudbees.groovy.cps.MethodLocation;
import com.cloudbees.groovy.cps.Outcome;
import com.cloudbees.groovy.cps.impl.Caller;
import com.cloudbees.groovy.cps.impl.CpsCallableInvocation;
import com.cloudbees.groovy.cps.impl.CpsFunction;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.SettableFuture;
import groovy.lang.Closure;
import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
import org.jenkinsci.plugins.workflow.steps.StepExecution;

Expand All @@ -36,12 +43,17 @@
import javax.annotation.Nullable;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.logging.Logger;

import static java.util.logging.Level.*;
import org.codehaus.groovy.runtime.DefaultGroovyMethods;
import org.codehaus.groovy.runtime.GroovyCategorySupport;
import org.codehaus.groovy.runtime.InvokerHelper;
import org.jenkinsci.plugins.workflow.cps.persistence.IteratorHack;
import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
import org.jenkinsci.plugins.workflow.support.concurrent.Futures;
import org.jenkinsci.plugins.workflow.support.concurrent.Timeout;
Expand Down Expand Up @@ -146,10 +158,33 @@ public StepExecution getStep() {
this.step = step;
}

/** TODO pending full coverage in {@link CpsDefaultGroovyMethods} */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such as is added in cloudbees/groovy-cps#52, but that mixes in other stuff which we do not need.

public static class CpsDefaultGroovyMethodsExt {
private static MethodLocation loc(String methodName) {
return new MethodLocation(CpsDefaultGroovyMethodsExt.class, methodName);
}
public static <T> List<T> each(List<T> self, Closure<?> closure) {
if (!Caller.isAsynchronous(self, "each", closure) &&
!Caller.isAsynchronous(CpsDefaultGroovyMethodsExt.class, "each", self, closure)) {
return DefaultGroovyMethods.each(self, closure);
}
Builder b = new Builder(loc("each"));
CpsFunction f = new CpsFunction(Arrays.asList("self", "closure"), b.block(
b.staticCall(-1, CpsDefaultGroovyMethods.class, "each",
b.staticCall(-1, InvokerHelper.class, "asIterator",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just delegates to the Iterator overload.

b.localVariable("self")),
b.localVariable("closure")),
b.return_(b.localVariable("self"))));
throw new CpsCallableInvocation(f, null, self, closure);
}
private CpsDefaultGroovyMethodsExt() {}
}

/**
* Executes CPS code synchronously a little bit more, until it hits
* the point the workflow needs to be dehydrated.
*/
@SuppressWarnings("rawtypes")
@Nonnull Outcome runNextChunk() {
assert program!=null;

Expand All @@ -160,9 +195,13 @@ public StepExecution getStep() {

try (Timeout timeout = Timeout.limit(5, TimeUnit.MINUTES)) {
LOGGER.log(FINE, "runNextChunk on {0}", resumeValue);
Outcome o = resumeValue;
final Outcome o = resumeValue;
resumeValue = null;
outcome = program.run0(o);
outcome = GroovyCategorySupport.use(Arrays.<Class>asList(CpsDefaultGroovyMethods.class, CpsDefaultGroovyMethodsExt.class, IteratorHack.class), new Closure<Outcome>(null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the black magic here, it'd be nice to have some comments explaining what exactly is going on. =)

@Override public Outcome call() {
return program.run0(o);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for experimentation; probably we would rather want to have a change in groovy-cps which

  • fleshes out CpsDefaultGroovyMethods
  • calls GroovyCategorySupport.use from inside Continuable.run0

}
});
if (outcome.getAbnormal() != null) {
LOGGER.log(FINE, "ran and produced error", outcome.getAbnormal());
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;

import org.codehaus.groovy.runtime.DefaultGroovyMethods;
import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
Expand Down Expand Up @@ -85,9 +84,11 @@ private boolean checkJenkins26481(Object[] args, /* TODO Java 8: just take Execu
if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures
return true;
}
for (int i = 0; i < args.length; i++) {
if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) {
throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops");
if (false) { // TODO be more discriminating
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppressing the main change in #15 for the moment; #14 has something closer to what we need, though ideally we would check the actual method and determine if we have CPS coverage for that method (and overload) now.

for (int i = 0; i < args.length; i++) {
if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) {
throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops");
}
}
}
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,91 +24,86 @@

package org.jenkinsci.plugins.workflow.cps.persistence;

import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.thoughtworks.xstream.converters.reflection.ReflectionProvider;
import hudson.Extension;
import java.lang.reflect.Field;
import com.cloudbees.groovy.cps.impl.Caller;
import java.io.Serializable;
import java.util.AbstractList;
import java.util.HashMap;
import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Collection;
import java.util.ConcurrentModificationException;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.workflow.pickles.Pickle;
import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
import java.util.NoSuchElementException;

/**
* Makes Java iterators effectively serializable.
*/
@Extension public class IteratorHack extends PickleFactory {
public class IteratorHack {

private static final Logger LOGGER = Logger.getLogger(IteratorHack.class.getName());
private static final Set<String> ACCEPTED_TYPES = ImmutableSet.of("java.util.ArrayList$Itr");

@Override public Pickle writeReplace(Object object) {
if (ACCEPTED_TYPES.contains(object.getClass().getName())) {
return new Replacement(object);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else {
return null;
/**
* Similar to the inner class of {@link AbstractList} except it is serializable.
* Since {@link AbstractList#modCount} is {@code transient} we cannot rely on it and thus cannot throw {@link ConcurrentModificationException}.
*/
private static final class Itr<E> implements Iterator<E>, Serializable {
private static final long serialVersionUID = 1;
private final List<E> list;
private int cursor = 0;
private int lastRet = -1;
Itr(List<E> list) {
this.list = list;
}
}

@SuppressWarnings("rawtypes")
private static class Replacement extends Pickle {

private final Class type;
private final Map<Class,Map<String,Object>> fields;

Replacement(Object o) {
type = o.getClass();
fields = new HashMap<>();
ReflectionProvider rp = Jenkins.XSTREAM2.getReflectionProvider();
rp.visitSerializableFields(o, new ReflectionProvider.Visitor() {
@Override public void visit(String name, Class type, Class definedIn, Object value) {
if (name.equals("expectedModCount")) {
LOGGER.log(Level.FINER, "ignoring expectedModCount={0}", value);
return;
}
Map<String,Object> fieldsByClass = fields.get(definedIn);
if (fieldsByClass == null) {
fieldsByClass = new HashMap<>();
fields.put(definedIn, fieldsByClass);
}
fieldsByClass.put(name, value);
}
});
LOGGER.log(Level.FINE, "replacing {0} with {1}", new Object[] {o, fields});
@Override public boolean hasNext() {
return cursor != list.size();
}

@Override public ListenableFuture<?> rehydrate() {
ReflectionProvider rp = Jenkins.XSTREAM2.getReflectionProvider();
Object o = rp.newInstance(type);
for (Map.Entry<Class,Map<String,Object>> entry : fields.entrySet()) {
Class definedIn = entry.getKey();
for (Map.Entry<String,Object> entry2 : entry.getValue().entrySet()) {
String fieldName = entry2.getKey();
Object value = entry2.getValue();
rp.writeField(o, fieldName, value, definedIn);
if (fieldName.equals("this$0")) {
try {
Field f = AbstractList.class.getDeclaredField("modCount"); // TODO if not handling only ArrayList.Itr, search in superclasses
f.setAccessible(true);
int modCount = f.getInt(value);
LOGGER.log(Level.FINER, "found a modCount={0}", modCount);
rp.writeField(o, "expectedModCount", modCount, type); // TODO search in superclasses
} catch (Exception x) {
return Futures.immediateFailedFuture(x);
}
}
@Override public E next() {
try {
int i = cursor;
E next = list.get(i);
lastRet = i;
cursor = i + 1;
return next;
} catch (IndexOutOfBoundsException e) {
throw new NoSuchElementException();
}
}
@Override public void remove() {
if (lastRet < 0) {
throw new IllegalStateException();
}
try {
list.remove(lastRet);
if (lastRet < cursor) {
cursor--;
}
lastRet = -1;
} catch (IndexOutOfBoundsException e) {
throw new ConcurrentModificationException();
}
LOGGER.log(Level.FINE, "reconstructed {0}", o);
return Futures.immediateFuture(o);
}
}

/** Serializable replacement for {@link List#iterator}. */
public static <E> Iterator<E> iterator(List<E> list) {
// TODO !Caller.isAsynchronous(list, "iterator") && !Caller.isAsynchronous(IteratorHack.class, "iterator", list) when used from a Java 5-style for-loop, so not sure how to sidestep this when running in @NonCPS
return new Itr<>(list);
}

/** Serializable replacement for {@link Map#entrySet}. */
public static <K, V> Collection<Map.Entry<K, V>> entrySet(Map<K, V> map) {
if (!Caller.isAsynchronous(map, "entrySet") && !Caller.isAsynchronous(IteratorHack.class, "entrySet", map)) {
// In @NonCPS so no need to bother processing this.
return map.entrySet();
}
// TODO return an actual Set
List<Map.Entry<K, V>> entries = new ArrayList<>();
for (Map.Entry<K, V> entry : map.entrySet()) {
// TODO return an object holding references to the map and the key and delegating all calls accordingly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These impls are not intended to be perfect or complete, just good enough to let typical script usages pass, where previously they would not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget - does this all enable for (a : listOfA) { ... } and the like?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see IteratorHackTest. Prior to this PR, ArrayList.iterator was safe but nothing else (and in particular I had no idea how to support iterating Maps and the like—the trick did not generalize well). Now most common interface methods in this category should work, and on any implementation class.

entries.add(new AbstractMap.SimpleImmutableEntry<>(entry));
}
return entries;
}

private IteratorHack() {}

}
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,8 @@ public void stop(Throwable cause) throws Exception {
SemaphoreStep.success("new-one/1", null);
SemaphoreStep.success("new-two/1", null);
story.j.waitForCompletion(b);
/* TODO desired behavior:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can have the “desired behavior” from #77.

story.j.assertBuildStatusSuccess(b);
story.j.assertLogContains("running new-style loop on two -> 2", b);
*/
story.j.assertBuildStatus(Result.FAILURE, b);
story.j.assertLogContains("java.io.NotSerializableException: java.util.LinkedHashMap$Entry", b);
}
});
}
Expand All @@ -247,7 +243,6 @@ public void stop(Throwable cause) throws Exception {
});
}

@Ignore("TODO backed out for JENKINS-34064")
@Issue("JENKINS-26481")
@Test public void eachClosure() {
story.addStep(new Statement() {
Expand Down