[JENKINS-27421] Categories also seem to provide a far simpler way to …

…work around unserializable iterators, map entries, etc.

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

@@ -53,6 +53,7 @@
 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;
@@ -196,7 +197,7 @@ private CpsDefaultGroovyMethodsExt() {}
             LOGGER.log(FINE, "runNextChunk on {0}", resumeValue);
             final Outcome o = resumeValue;
             resumeValue = null;
-            outcome = GroovyCategorySupport.use(Arrays.<Class>asList(CpsDefaultGroovyMethods.class, CpsDefaultGroovyMethodsExt.class), new Closure<Outcome>(null) {
+            outcome = GroovyCategorySupport.use(Arrays.<Class>asList(CpsDefaultGroovyMethods.class, CpsDefaultGroovyMethodsExt.class, IteratorHack.class), new Closure<Outcome>(null) {
                 @Override public Outcome call() {
                     return program.run0(o);
                 }

src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java

@@ -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);
-        } 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
+            entries.add(new AbstractMap.SimpleImmutableEntry<>(entry));
+        }
+        return entries;
     }
 
+    private IteratorHack() {}
+
 }

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

@@ -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:
                 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);
             }
         });
     }