[JENKINS-27421] Test for LinkedHashMap.Entry problem

[jglick]

JENKINS-27421

Tried to solve using

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
index f7f6ab3..355e478 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
@@ -46,7 +46,11 @@ import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
 @Extension public class IteratorHack extends PickleFactory {

     private static final Logger LOGGER = Logger.getLogger(IteratorHack.class.getName());
-    private static final Set<String> ACCEPTED_TYPES = ImmutableSet.of("java.util.ArrayList$Itr");
+    private static final Set<String> ACCEPTED_TYPES = ImmutableSet.of(
+        "java.util.ArrayList$Itr",
+        "java.util.LinkedHashMap$Entry",
+        "java.util.LinkedHashMap$LinkedEntryIterator",
+        "java.util.LinkedHashMap$LinkedEntrySet");

     @Override public Pickle writeReplace(Object object) {
         if (ACCEPTED_TYPES.contains(object.getClass().getName())) {

but this does not seem to work, I think because Entry has self-links.

@reviewbybees

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[jglick]

The problem seems to be that Pickles cannot refer to other objects which also need pickling; they must be serializable in isolation.

I tried extending PickleFactory to allow you to specify plain objects (not Pickles) which would be deserialized using readResolve:

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/pickles/PickleFactory.java b/src/main/java/org/jenkinsci/plugins/workflow/pickles/PickleFactory.java
index fd993c7..18f3320 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/pickles/PickleFactory.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/pickles/PickleFactory.java
@@ -35,7 +35,13 @@ import javax.annotation.Nonnull;
  */
 public abstract class PickleFactory implements ExtensionPoint {

-    public abstract @CheckForNull Pickle writeReplace(@Nonnull Object object);
+    public @CheckForNull Pickle writeReplace(@Nonnull Object object) {
+        return null;
+    }
+
+    public @CheckForNull Object simpleWriteReplace(@Nonnull Object object) {
+        return null;
+    }

     public static ExtensionList<PickleFactory> all() {
         return ExtensionList.lookup(PickleFactory.class);

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/serialization/RiverWriter.java b/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/serialization/RiverWriter.java
index 1d18130..a5ff31c 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/serialization/RiverWriter.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/support/pickles/serialization/RiverWriter.java
@@ -119,6 +119,12 @@ public class RiverWriter implements Closeable {
                         }
                     }
                 }
+                for (PickleFactory f : pickleFactories) {
+                    Object o2 = f.simpleWriteReplace(o);
+                    if (o2 != null) {
+                        return o2;
+                    }
+                }
                 return o;
             }
         });

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
index f7f6ab3..418ec7b 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java
@@ -25,10 +25,9 @@
 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.io.Serializable;
 import java.lang.reflect.Field;
 import java.util.AbstractList;
 import java.util.HashMap;
@@ -37,7 +36,6 @@ 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;

 /**
@@ -46,9 +44,13 @@ import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
 @Extension public class IteratorHack extends PickleFactory {

     private static final Logger LOGGER = Logger.getLogger(IteratorHack.class.getName());
-    private static final Set<String> ACCEPTED_TYPES = ImmutableSet.of("java.util.ArrayList$Itr");
+    private static final Set<String> ACCEPTED_TYPES = ImmutableSet.of(
+        "java.util.ArrayList$Itr",
+        "java.util.LinkedHashMap$Entry",
+        "java.util.LinkedHashMap$LinkedEntryIterator",
+        "java.util.LinkedHashMap$LinkedEntrySet");

-    @Override public Pickle writeReplace(Object object) {
+    @Override public Object simpleWriteReplace(Object object) {
         if (ACCEPTED_TYPES.contains(object.getClass().getName())) {
             return new Replacement(object);
         } else {
@@ -57,7 +59,7 @@ import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
     }

     @SuppressWarnings("rawtypes")
-    private static class Replacement extends Pickle {
+    private static class Replacement implements Serializable {

         private final Class type;
         private final Map<Class,Map<String,Object>> fields;
@@ -83,7 +85,7 @@ import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
             LOGGER.log(Level.FINE, "replacing {0} with {1}", new Object[] {o, fields});
         }

-        @Override public ListenableFuture<?> rehydrate() {
+        private Object readResolve() throws Exception {
             ReflectionProvider rp = Jenkins.XSTREAM2.getReflectionProvider();
             Object o = rp.newInstance(type);
             for (Map.Entry<Class,Map<String,Object>> entry : fields.entrySet()) {
@@ -93,20 +95,16 @@ import org.jenkinsci.plugins.workflow.pickles.PickleFactory;
                     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);
-                        }
                     }
                 }
             }
             LOGGER.log(Level.FINE, "reconstructed {0}", o);
-            return Futures.immediateFuture(o);
+            return o;
         }

     }

which does prevent the NotSerializableException, but causes the test to hang in a bizarre way: after going through both C-style iterations, and the first new-style iteration, the program seems to try to go back to the second C-style iteration:

=== Starting mapIterator(org.jenkinsci.plugins.workflow.SerializationTest)
Unblocking new-one/1 as success
Planning to unblock new-two/1 as success
Blocking C-two/2
[demo #1] [Pipeline] echo
[demo #1] running C-style loop on two → 2
[demo #1] [Pipeline] semaphore

I assume this is somehow caused by object references being mangled. @kohsuke any ideas?

[abayer]

🐝