WFLY-11585 Access ordered LinkedHashMap not externalized correctly

clustering/marshalling/spi/src/main/java/org/wildfly/clustering/marshalling/spi/DefaultExternalizer.java

@@ -44,7 +44,6 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.Locale;
@@ -80,8 +79,9 @@
 import org.wildfly.clustering.marshalling.spi.util.CollectionExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.CopyOnWriteCollectionExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.DateExternalizer;
+import org.wildfly.clustering.marshalling.spi.util.HashMapExternalizer;
+import org.wildfly.clustering.marshalling.spi.util.LinkedHashMapExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.MapEntryExternalizer;
-import org.wildfly.clustering.marshalling.spi.util.MapExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.SingletonCollectionExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.SingletonMapExternalizer;
 import org.wildfly.clustering.marshalling.spi.util.SortedMapExternalizer;
@@ -122,7 +122,7 @@ public enum DefaultExternalizer implements Externalizer<Object> {
     ATOMIC_LONG(new LongExternalizer<>(AtomicLong.class, AtomicLong::new, AtomicLong::get)),
     ATOMIC_REFERENCE(new ObjectExternalizer<>(AtomicReference.class, AtomicReference::new, AtomicReference::get)),
     CALENDAR(new CalendarExternalizer()),
-    CONCURRENT_HASH_MAP(new MapExternalizer<>(ConcurrentHashMap.class, ConcurrentHashMap::new)),
+    CONCURRENT_HASH_MAP(new HashMapExternalizer<>(ConcurrentHashMap.class, ConcurrentHashMap::new)),
     CONCURRENT_HASH_SET(new CollectionExternalizer<>(ConcurrentHashMap.KeySetView.class, ConcurrentHashMap::newKeySet)),
     CONCURRENT_LINKED_DEQUE(new CollectionExternalizer<>(ConcurrentLinkedDeque.class, size -> new ConcurrentLinkedDeque<>())),
     CONCURRENT_LINKED_QUEUE(new CollectionExternalizer<>(ConcurrentLinkedQueue.class, size -> new ConcurrentLinkedQueue<>())),
@@ -142,9 +142,9 @@ public enum DefaultExternalizer implements Externalizer<Object> {
     EMPTY_SET(new ValueExternalizer<>(Collections.emptySet())),
     EMPTY_SORTED_MAP(new ValueExternalizer<>(Collections.emptySortedMap())),
     EMPTY_SORTED_SET(new ValueExternalizer<>(Collections.emptySortedSet())),
-    HASH_MAP(new MapExternalizer<>(HashMap.class, HashMap::new)),
+    HASH_MAP(new HashMapExternalizer<>(HashMap.class, HashMap::new)),
     HASH_SET(new CollectionExternalizer<>(HashSet.class, HashSet::new)),
-    LINKED_HASH_MAP(new MapExternalizer<>(LinkedHashMap.class, LinkedHashMap::new)),
+    LINKED_HASH_MAP(new LinkedHashMapExternalizer()),
     LINKED_HASH_SET(new CollectionExternalizer<>(LinkedHashSet.class, LinkedHashSet::new)),
     LINKED_LIST(new CollectionExternalizer<>(LinkedList.class, size -> new LinkedList<>())),
     LOCALE(new StringExternalizer<>(Locale.class, Locale::forLanguageTag, Locale::toLanguageTag)),

clustering/marshalling/spi/src/main/java/org/wildfly/clustering/marshalling/spi/util/HashMapExternalizer.java

@@ -0,0 +1,53 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2019, Red Hat, Inc., and individual contributors
+ * as indicated by the @author tags. See the copyright.txt file in the
+ * distribution for a full listing of individual contributors.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.wildfly.clustering.marshalling.spi.util;
+
+import java.io.ObjectInput;
+import java.io.ObjectOutput;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+/**
+ * @author Paul Ferraro
+ */
+public class HashMapExternalizer<T extends Map<Object, Object>> extends MapExternalizer<T, Void> {
+
+    public HashMapExternalizer(Class<?> targetClass, Supplier<T> factory) {
+        super(targetClass, new Function<Void, T>() {
+            @Override
+            public T apply(Void context) {
+                return factory.get();
+            }
+        });
+    }
+
+    @Override
+    protected void writeContext(ObjectOutput output, T map) {
+    }
+
+    @Override
+    protected Void readContext(ObjectInput input) {
+        return null;
+    }
+}

clustering/marshalling/spi/src/main/java/org/wildfly/clustering/marshalling/spi/util/LinkedHashMapExternalizer.java

@@ -0,0 +1,71 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2019, Red Hat, Inc., and individual contributors
+ * as indicated by the @author tags. See the copyright.txt file in the
+ * distribution for a full listing of individual contributors.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.wildfly.clustering.marshalling.spi.util;
+
+import java.io.IOException;
+import java.io.ObjectInput;
+import java.io.ObjectOutput;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.function.Function;
+
+/**
+ * @author Paul Ferraro
+ */
+public class LinkedHashMapExternalizer extends MapExternalizer<LinkedHashMap<Object, Object>, Boolean> {
+
+    public LinkedHashMapExternalizer() {
+        super(LinkedHashMap.class, new Function<Boolean, LinkedHashMap<Object, Object>>() {
+            @Override
+            public LinkedHashMap<Object, Object> apply(Boolean accessOrder) {
+                // Use capacity and load factor defaults
+                return new LinkedHashMap<>(16, 0.75f, accessOrder);
+            }
+        });
+    }
+
+    @Override
+    protected void writeContext(ObjectOutput output, LinkedHashMap<Object, Object> map) throws IOException {
+        Object insertOrder = new Object();
+        Object accessOrder = new Object();
+        map.put(insertOrder, null);
+        map.put(accessOrder, null);
+        // Access first inserted entry
+        // If map uses access order, this element will move to the tail of the map
+        map.get(insertOrder);
+        Iterator<Object> keys = map.keySet().iterator();
+        Object element = keys.next();
+        while ((element != insertOrder) && (element != accessOrder)) {
+            element = keys.next();
+        }
+        map.remove(insertOrder);
+        map.remove(accessOrder);
+        // Map uses access order if previous access changed iteration order
+        output.writeBoolean(element == accessOrder);
+    }
+
+    @Override
+    protected Boolean readContext(ObjectInput input) throws IOException {
+        return input.readBoolean();
+    }
+}

clustering/marshalling/spi/src/main/java/org/wildfly/clustering/marshalling/spi/util/MapExternalizer.java

@@ -26,7 +26,7 @@
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
 import java.util.Map;
-import java.util.function.IntFunction;
+import java.util.function.Function;
 
 import org.wildfly.clustering.marshalling.Externalizer;
 import org.wildfly.clustering.marshalling.spi.IndexSerializer;
@@ -35,23 +35,20 @@
  * Externalizers for implementations of {@link Map}.
  * @author Paul Ferraro
  */
-public class MapExternalizer<T extends Map<Object, Object>> implements Externalizer<T> {
+public abstract class MapExternalizer<T extends Map<Object, Object>, C> implements Externalizer<T> {
 
     private final Class<T> targetClass;
-    private final IntFunction<T> factory;
+    private final Function<C, T> factory;
 
     @SuppressWarnings("unchecked")
-    public MapExternalizer(Class<?> targetClass, IntFunction<T> factory) {
+    protected MapExternalizer(Class<?> targetClass, Function<C, T> factory) {
         this.targetClass = (Class<T>) targetClass;
         this.factory = factory;
     }
 
     @Override
     public void writeObject(ObjectOutput output, T map) throws IOException {
-        writeMap(output, map);
-    }
-
-    static <T extends Map<Object, Object>> void writeMap(ObjectOutput output, T map) throws IOException {
+        this.writeContext(output, map);
         IndexSerializer.VARIABLE.writeInt(output, map.size());
         for (Map.Entry<Object, Object> entry : map.entrySet()) {
             output.writeObject(entry.getKey());
@@ -61,11 +58,9 @@ static <T extends Map<Object, Object>> void writeMap(ObjectOutput output, T map)
 
     @Override
     public T readObject(ObjectInput input) throws IOException, ClassNotFoundException {
+        C context = this.readContext(input);
+        T map = this.factory.apply(context);
         int size = IndexSerializer.VARIABLE.readInt(input);
-        return readMap(input, this.factory.apply(size), size);
-    }
-
-    static <T extends Map<Object, Object>> T readMap(ObjectInput input, T map, int size) throws IOException, ClassNotFoundException {
         for (int i = 0; i < size; ++i) {
             map.put(input.readObject(), input.readObject());
         }
@@ -76,4 +71,21 @@ static <T extends Map<Object, Object>> T readMap(ObjectInput input, T map, int s
     public Class<T> getTargetClass() {
         return this.targetClass;
     }
+
+    /**
+     * Writes the context of the specified map to the specified output stream.
+     * @param output an output stream
+     * @param map the target map
+     * @throws IOException if the constructor context cannot be written to the stream
+     */
+    protected abstract void writeContext(ObjectOutput output, T map) throws IOException;
+
+    /**
+     * Reads the map context from the specified input stream.
+     * @param input an input stream
+     * @return the map constructor context
+     * @throws IOException if the constructor context cannot be read from the stream
+     * @throws ClassNotFoundException if a class could not be found
+     */
+    protected abstract C readContext(ObjectInput input) throws IOException, ClassNotFoundException;
 }

clustering/marshalling/spi/src/main/java/org/wildfly/clustering/marshalling/spi/util/SortedMapExternalizer.java

@@ -29,41 +29,25 @@
 import java.util.SortedMap;
 import java.util.function.Function;
 
-import org.wildfly.clustering.marshalling.Externalizer;
-import org.wildfly.clustering.marshalling.spi.IndexSerializer;
-
 /**
  * Externalizers for implementations of {@link SortedMap}.
  * Requires additional serialization of the comparator.
  * @author Paul Ferraro
  */
-public class SortedMapExternalizer<T extends SortedMap<Object, Object>> implements Externalizer<T> {
-
-    private final Class<T> targetClass;
-    private final Function<Comparator<? super Object>, T> factory;
+public class SortedMapExternalizer<T extends SortedMap<Object, Object>> extends MapExternalizer<T, Comparator<Object>> {
 
-    @SuppressWarnings("unchecked")
-    public SortedMapExternalizer(Class<?> targetClass, Function<Comparator<? super Object>, T> factory) {
-        this.targetClass = (Class<T>) targetClass;
-        this.factory = factory;
+    public SortedMapExternalizer(Class<?> targetClass, Function<Comparator<Object>, T> factory) {
+        super(targetClass, factory);
     }
 
     @Override
-    public void writeObject(ObjectOutput output, T map) throws IOException {
+    protected void writeContext(ObjectOutput output, T map) throws IOException {
         output.writeObject(map.comparator());
-        MapExternalizer.writeMap(output, map);
-    }
-
-    @Override
-    public T readObject(ObjectInput input) throws IOException, ClassNotFoundException {
-        @SuppressWarnings("unchecked")
-        Comparator<? super Object> comparator = (Comparator<? super Object>) input.readObject();
-        int size = IndexSerializer.VARIABLE.readInt(input);
-        return MapExternalizer.readMap(input, this.factory.apply(comparator), size);
     }
 
+    @SuppressWarnings("unchecked")
     @Override
-    public Class<T> getTargetClass() {
-        return this.targetClass;
+    protected Comparator<Object> readContext(ObjectInput input) throws IOException, ClassNotFoundException {
+        return (Comparator<Object>) input.readObject();
     }
 }

clustering/marshalling/spi/src/test/java/org/wildfly/clustering/marshalling/spi/util/MapExternalizerTestCase.java

@@ -25,6 +25,7 @@
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.NavigableMap;
@@ -52,7 +53,10 @@ public void test() throws ClassNotFoundException, IOException {
         Map<Object, Object> basis = Stream.of(1, 2, 3, 4, 5).collect(Collectors.<Integer, Object, Object>toMap(i -> i, i -> Integer.toString(i)));
         new ExternalizerTester<>(DefaultExternalizer.CONCURRENT_HASH_MAP.cast(ConcurrentHashMap.class), MapExternalizerTestCase::assertMapEquals).test(new ConcurrentHashMap<>(basis));
         new ExternalizerTester<>(DefaultExternalizer.HASH_MAP.cast(HashMap.class), MapExternalizerTestCase::assertMapEquals).test(new HashMap<>(basis));
-        new ExternalizerTester<>(DefaultExternalizer.LINKED_HASH_MAP.cast(LinkedHashMap.class), MapExternalizerTestCase::assertMapEquals).test(new LinkedHashMap<>(basis));
+        new ExternalizerTester<>(DefaultExternalizer.LINKED_HASH_MAP.cast(LinkedHashMap.class), MapExternalizerTestCase::assertLinkedMapEquals).test(new LinkedHashMap<>(basis));
+        LinkedHashMap<Object, Object> accessOrderMap = new LinkedHashMap<>(5, 1, true);
+        accessOrderMap.putAll(basis);
+        new ExternalizerTester<>(DefaultExternalizer.LINKED_HASH_MAP.cast(LinkedHashMap.class), MapExternalizerTestCase::assertLinkedMapEquals).test(accessOrderMap);
 
         new ExternalizerTester<>(DefaultExternalizer.EMPTY_MAP.cast(Map.class), Assert::assertSame).test(Collections.emptyMap());
         new ExternalizerTester<>(DefaultExternalizer.EMPTY_NAVIGABLE_MAP.cast(NavigableMap.class), Assert::assertSame).test(Collections.emptyNavigableMap());
@@ -71,4 +75,19 @@ static <T extends Map<Object, Object>> void assertMapEquals(T expected, T actual
             Assert.assertEquals(entry.getValue(), actual.get(entry.getKey()));
         }
     }
+
+    static <T extends Map<Object, Object>> void assertLinkedMapEquals(T expected, T actual) {
+        Assert.assertEquals(expected.size(), actual.size());
+        // Change access order
+        expected.get(expected.keySet().iterator().next());
+        actual.get(actual.keySet().iterator().next());
+        Iterator<Map.Entry<Object, Object>> expectedEntries = expected.entrySet().iterator();
+        Iterator<Map.Entry<Object, Object>> actualEntries = actual.entrySet().iterator();
+        while (expectedEntries.hasNext() && actualEntries.hasNext()) {
+            Map.Entry<Object, Object> expectedEntry = expectedEntries.next();
+            Map.Entry<Object, Object> actualEntry = actualEntries.next();
+            Assert.assertEquals(expectedEntry.getKey(), actualEntry.getKey());
+            Assert.assertEquals(expectedEntry.getValue(), actualEntry.getValue());
+        }
+    }
 }