From cbb7d010182e2caee0ec5799fce5b98f70f29166 Mon Sep 17 00:00:00 2001 From: piotr-duzniak_tisint Date: Thu, 30 Jul 2026 14:09:50 +0200 Subject: [PATCH 1/2] fix(java): reproduce silent Set child-loss for package-private nodes in cyclic graphs Wide fan-out (one parent, many children) cyclic graph of package-private nodes loses entries from a Set field on deserialize - no exception, unlike the codegen CompileException PackagePrivateMapKeyTest guards against. Reproduced from a production graph with 100+ children under one node; the child->parent back-reference (EnumMap-keyed) survives the round trip, the parent->child Set does not. --- .../PackagePrivateCyclicSetChildLossTest.java | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java diff --git a/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java b/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java new file mode 100644 index 0000000000..aa92eb0266 --- /dev/null +++ b/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.codegen.pkgprivate; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import org.apache.fory.Fory; +import org.apache.fory.ThreadSafeFory; +import org.testng.annotations.Test; + +/** + * Regression test: a wide fan-out (one parent, many children) cyclic graph of package-private + * nodes silently loses entries from a {@code Set} field during + * deserialization - no exception is thrown, so unlike {@link PackagePrivateMapKeyTest} (which + * catches the codegen CompileException for this same field shape) this defect only surfaces as + * missing data. + * + *

Reproduced from a production graph with 100+ children under one parent node; the + * child->parent edge (an EnumMap-keyed back-reference) survives the round trip intact, but the + * parent->child edge (a plain HashSet) loses a handful of entries. Both edges are set together, + * atomically, at construction time, so the source object graph itself is never inconsistent - + * the asymmetry is introduced purely by fory's serialize/deserialize round trip. + */ +public class PackagePrivateCyclicSetChildLossTest { + + private static final int CHILD_COUNT = 200; + + @Test + public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() { + ThreadSafeFory fury = + Fory.builder() + .withXlang(false) + .requireClassRegistration(false) + .withRefTracking(true) + .withCompatible(false) + .buildThreadSafeFory(); + + FanOutContainer container = new FanOutContainer("v1"); + FanOutNode parent = new FanOutNode(FanOutType.TYPE_A, "parent"); + container.nodes.computeIfAbsent(FanOutType.TYPE_A, k -> new HashMap<>()).put(parent.id, parent); + + List expectedChildIds = new ArrayList<>(); + for (int i = 0; i < CHILD_COUNT; i++) { + FanOutNode child = new FanOutNode(FanOutType.TYPE_B, "child-" + i); + parent.children.add(child); + child.parents.computeIfAbsent(parent.type, k -> new LinkedHashSet<>()).add(parent); + container.nodes.computeIfAbsent(FanOutType.TYPE_B, k -> new HashMap<>()).put(child.id, child); + expectedChildIds.add(child.id); + } + + byte[] bytes = fury.serialize(container); + FanOutContainer result = (FanOutContainer) fury.deserialize(bytes); + + FanOutNode resultParent = result.nodes.get(FanOutType.TYPE_A).get("parent"); + assertEquals(resultParent.children.size(), CHILD_COUNT, "parent lost children from its Set field"); + + // Every child must still be reachable BOTH ways: down (parent.children) and up + // (child.parents), and both directions must point at the exact same object (fory's + // refTracking should unify identical (type,id) instances, not duplicate them). + List asymmetric = new ArrayList<>(); + for (String childId : expectedChildIds) { + FanOutNode resultChild = result.nodes.get(FanOutType.TYPE_B).get(childId); + boolean parentListsChild = resultParent.children.contains(resultChild); + boolean childListsParent = + resultChild.parents.getOrDefault(FanOutType.TYPE_A, Set.of()).contains(resultParent); + if (!parentListsChild || !childListsParent) { + asymmetric.add( + childId + " (parentListsChild=" + parentListsChild + ", childListsParent=" + childListsParent + ")"); + } + } + assertTrue( + asymmetric.isEmpty(), + "asymmetric parent<->child edges after round trip (should be empty): " + asymmetric); + } +} + +// All package-private — this triggers the bug +enum FanOutType implements Serializable { + TYPE_A, + TYPE_B +} + +class FanOutNode implements Serializable { + final FanOutType type; + final String id; + final Set children = new HashSet<>(); + final Map> parents = new EnumMap<>(FanOutType.class); + + FanOutNode(FanOutType type, String id) { + this.type = type; + this.id = id; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof FanOutNode)) { + return false; + } + FanOutNode other = (FanOutNode) o; + return type == other.type && id.equals(other.id); + } + + @Override + public int hashCode() { + return Objects.hash(type, id); + } +} + +class FanOutContainer implements Serializable { + final Map> nodes = new EnumMap<>(FanOutType.class); + final String version; + + FanOutContainer(String version) { + this.version = version; + } +} From 54c15f0cdb8298f9b97ed7b6fd18c178d6921d72 Mon Sep 17 00:00:00 2001 From: chaokunyang Date: Sat, 1 Aug 2026 20:01:46 +0800 Subject: [PATCH 2/2] fix(java): repair cyclic hash container lookups --- .../fory/builder/BaseObjectCodecBuilder.java | 55 +++++- .../org/apache/fory/context/MapRefReader.java | 65 ++++++- .../org/apache/fory/context/RefReader.java | 28 ++- .../collection/CollectionLikeSerializer.java | 28 ++- .../collection/HashContainerReadState.java | 111 ++++++++++++ .../collection/MapLikeSerializer.java | 27 ++- .../PackagePrivateCyclicSetChildLossTest.java | 43 +++-- .../collection/CyclicHashContainerTest.java | 164 ++++++++++++++++++ 8 files changed, 489 insertions(+), 32 deletions(-) create mode 100644 java/fory-core/src/main/java/org/apache/fory/serializer/collection/HashContainerReadState.java create mode 100644 java/fory-core/src/test/java/org/apache/fory/serializer/collection/CyclicHashContainerTest.java diff --git a/java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java b/java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java index 7bdda7394c..6fb95cfb55 100644 --- a/java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java +++ b/java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java @@ -2729,13 +2729,32 @@ protected Expression deserializeForCollection( serializer.type()); } Invoke supportHook = inlineInvoke(serializer, "supportCodegenHook", PRIMITIVE_BOOLEAN_TYPE); + Class collectionType = getRawType(typeRef); + boolean mayReadHashSet = + Set.class.isAssignableFrom(collectionType) || collectionType.isAssignableFrom(Set.class); Expression collection = new Invoke(serializer, "newCollection", COLLECTION_TYPE, readContextRef); Expression size = new Invoke(serializer, "getAndClearNumElements", "size", PRIMITIVE_INT_TYPE); // Do not add an ArrayList-specific branch here: it pushes generated code over 325 bytes, and // List#add is more likely to inline when the call site has only one receiver subclass. Expression hookRead = readCollectionCodegen(buffer, collection, size, elementType); - hookRead = new Invoke(serializer, "onCollectionRead", OBJECT_TYPE, hookRead); + Expression hashRefEpoch = null; + if (mayReadHashSet) { + hashRefEpoch = + new Invoke( + serializer, + "hashRefEpoch", + "hashRefEpoch", + PRIMITIVE_LONG_TYPE, + false, + false, + readContextRef); + hookRead = + new Invoke( + serializer, "onCollectionRead", OBJECT_TYPE, readContextRef, hookRead, hashRefEpoch); + } else { + hookRead = new Invoke(serializer, "onCollectionRead", OBJECT_TYPE, hookRead); + } Expression fallbackAction = read(serializer, buffer, OBJECT_TYPE); Expression fallbackRead = invokeGenerated( @@ -2744,8 +2763,11 @@ protected Expression deserializeForCollection( new ListExpression(fallbackAction, new Return(fallbackAction)), "readCollectionFallback", false); - Expression action = - new If(supportHook, new ListExpression(collection, hookRead), fallbackRead, false); + Expression hookAction = + mayReadHashSet + ? new ListExpression(hashRefEpoch, collection, hookRead) + : new ListExpression(collection, hookRead); + Expression action = new If(supportHook, hookAction, fallbackRead, false); if (invokeHint != null && invokeHint.genNewMethod) { invokeHint.add(buffer); invokeHint.add(readContextRef()); @@ -2986,6 +3008,21 @@ protected Expression deserializeForMap( Expression mapSerializer = serializer; Invoke supportHook = inlineInvoke(serializer, "supportCodegenHook", PRIMITIVE_BOOLEAN_TYPE); ListExpression expressions = new ListExpression(); + Class mapType = getRawType(typeRef); + boolean mayReadHashMap = + HashMap.class.isAssignableFrom(mapType) || mapType.isAssignableFrom(HashMap.class); + Expression hashRefEpoch = null; + if (mayReadHashMap) { + hashRefEpoch = + new Invoke( + serializer, + "hashRefEpoch", + "hashRefEpoch", + PRIMITIVE_LONG_TYPE, + false, + false, + readContextRef); + } Expression newMap = new Invoke(serializer, "newMap", MAP_TYPE, readContextRef); Expression size = new Invoke(serializer, "getAndClearNumElements", "size", PRIMITIVE_INT_TYPE); Expression chunkHeader = @@ -2993,6 +3030,9 @@ protected Expression deserializeForMap( eq(size, ofInt(0)), ofInt(0), inlineInvoke(buffer, "readUnsignedByte", PRIMITIVE_INT_TYPE)); + if (mayReadHashMap) { + expressions.add(hashRefEpoch); + } expressions.add(newMap, size, chunkHeader); Class keyCls = keyType.getRawType(); Class valueCls = valueType.getRawType(); @@ -3052,7 +3092,14 @@ chunkHeader, cast(bitand(sizeAndHeader2, ofInt(0xff)), PRIMITIVE_INT_TYPE)), invokeGenerated(ctx, chunkLoopCutPoints, chunksLoop, "readMapChunks", false); expressions.add(chunkLoopExpr, newMap); // first newMap to create map, last newMap as expr value - Expression map = inlineInvoke(serializer, "onMapRead", OBJECT_TYPE, expressions); + Expression map; + if (mayReadHashMap) { + map = + inlineInvoke( + serializer, "onMapRead", OBJECT_TYPE, readContextRef, expressions, hashRefEpoch); + } else { + map = inlineInvoke(serializer, "onMapRead", OBJECT_TYPE, expressions); + } Expression action = new If(supportHook, map, read(serializer, buffer, OBJECT_TYPE), false); if (invokeHint != null && invokeHint.genNewMethod) { invokeHint.add(buffer); diff --git a/java/fory-core/src/main/java/org/apache/fory/context/MapRefReader.java b/java/fory-core/src/main/java/org/apache/fory/context/MapRefReader.java index 59101b61ac..df068faebe 100644 --- a/java/fory-core/src/main/java/org/apache/fory/context/MapRefReader.java +++ b/java/fory-core/src/main/java/org/apache/fory/context/MapRefReader.java @@ -19,6 +19,7 @@ package org.apache.fory.context; +import java.util.Arrays; import org.apache.fory.Fory; import org.apache.fory.collection.IntArray; import org.apache.fory.collection.ObjectArray; @@ -38,14 +39,20 @@ public final class MapRefReader implements RefReader { private long readTotalObjectSize = 0; private final ObjectArray readObjects = new ObjectArray(DEFAULT_ARRAY_CAPACITY); private final IntArray readRefIds = new IntArray(DEFAULT_ARRAY_CAPACITY); + private boolean[] materializingRefs = new boolean[DEFAULT_ARRAY_CAPACITY]; private Object readObject; + private int materializingRefCount; + private long materializingRefEpoch; + private RefReadListener refReadListener; /** Reads a ref-or-null header and resolves cached references immediately when present. */ @Override public byte readRefOrNull(MemoryBuffer buffer) { byte headFlag = buffer.readByte(); if (headFlag == Fory.REF_FLAG) { - readObject = getReadRef(buffer.readVarUInt32Small14()); + int refId = buffer.readVarUInt32Small14(); + readObject = getReadRef(refId); + recordMaterializingRef(refId); } else { readObject = null; } @@ -73,7 +80,9 @@ public int preserveRefId(int refId) { public int tryPreserveRefId(MemoryBuffer buffer) { byte headFlag = buffer.readByte(); if (headFlag == Fory.REF_FLAG) { - readObject = getReadRef(buffer.readVarUInt32Small14()); + int refId = buffer.readVarUInt32Small14(); + readObject = getReadRef(refId); + recordMaterializingRef(refId); } else { readObject = null; if (headFlag == Fory.REF_VALUE_FLAG) { @@ -97,11 +106,20 @@ public boolean hasPreservedRefId() { return readRefIds.size > 0; } - /** Binds the most recently reserved ref id to {@code object}. */ + /** Early-publishes the most recently reserved ref id while its serializer is still reading. */ @Override public void reference(Object object) { int refId = readRefIds.pop(); - setReadRef(refId, object); + if (refId >= 0) { + readObjects.set(refId, object); + if (refId >= materializingRefs.length) { + materializingRefs = Arrays.copyOf(materializingRefs, readObjects.objects.length); + } + if (!materializingRefs[refId]) { + materializingRefs[refId] = true; + materializingRefCount++; + } + } } /** Returns the previously materialized object stored at {@code id}. */ @@ -116,11 +134,42 @@ public Object getReadRef() { return readObject; } - /** Stores {@code object} under an already reserved read ref id. */ + /** Stores the completed object and closes a matching early publication. */ @Override public void setReadRef(int id, Object object) { if (id >= 0) { readObjects.set(id, object); + if (id < materializingRefs.length && materializingRefs[id]) { + materializingRefs[id] = false; + materializingRefCount--; + if (materializingRefCount == 0 && refReadListener != null) { + refReadListener.onRefReadsComplete(); + } + } + } + } + + @Override + public long getMaterializingRefEpoch() { + return materializingRefEpoch; + } + + @Override + public boolean hasMaterializingRefs() { + return materializingRefCount != 0; + } + + @Override + public void setRefReadListener(RefReadListener listener) { + if (refReadListener != null && refReadListener != listener) { + throw new IllegalStateException("A read-reference listener is already installed"); + } + refReadListener = listener; + } + + private void recordMaterializingRef(int refId) { + if (refId < materializingRefs.length && materializingRefs[refId]) { + materializingRefEpoch++; } } @@ -146,6 +195,12 @@ public void reset() { } readObjects.clearApproximate(avg); readRefIds.clear(); + if (materializingRefCount != 0) { + Arrays.fill(materializingRefs, false); + } readObject = null; + materializingRefCount = 0; + materializingRefEpoch = 0; + refReadListener = null; } } diff --git a/java/fory-core/src/main/java/org/apache/fory/context/RefReader.java b/java/fory-core/src/main/java/org/apache/fory/context/RefReader.java index c295bedc38..e7b172b318 100644 --- a/java/fory-core/src/main/java/org/apache/fory/context/RefReader.java +++ b/java/fory-core/src/main/java/org/apache/fory/context/RefReader.java @@ -28,6 +28,11 @@ * materialized, and resolve previously read references by id. */ public interface RefReader { + /** Listener notified when every early-published read reference has finished materializing. */ + interface RefReadListener { + void onRefReadsComplete(); + } + /** Reads a ref-or-null header and returns the raw header byte. */ byte readRefOrNull(MemoryBuffer buffer); @@ -46,7 +51,10 @@ public interface RefReader { /** Returns whether there is a preserved id waiting to be bound to an object. */ boolean hasPreservedRefId(); - /** Binds the most recently preserved reference id to {@code object}. */ + /** + * Publishes {@code object} under the most recently preserved id before its read is complete. The + * outer read wrapper must later call {@link #setReadRef(int, Object)} for that id. + */ void reference(Object object); /** Returns the previously materialized object for a specific ref id. */ @@ -55,9 +63,25 @@ public interface RefReader { /** Returns the object resolved by the last ref-header read. */ Object getReadRef(); - /** Replaces the object stored for a previously preserved ref id. */ + /** Stores a completed object, ending any early publication for {@code id}. */ void setReadRef(int id, Object object); + /** + * Returns an operation-local epoch incremented when a back-reference resolves to an object that + * is still materializing. + */ + default long getMaterializingRefEpoch() { + return 0; + } + + /** Returns whether any early-published reference is still materializing. */ + default boolean hasMaterializingRefs() { + return false; + } + + /** Installs the single operation-local listener for reference materialization completion. */ + default void setRefReadListener(RefReadListener listener) {} + /** Clears all per-operation ref-tracking state. */ void reset(); diff --git a/java/fory-core/src/main/java/org/apache/fory/serializer/collection/CollectionLikeSerializer.java b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/CollectionLikeSerializer.java index b248c9bc65..32d46f973d 100644 --- a/java/fory-core/src/main/java/org/apache/fory/serializer/collection/CollectionLikeSerializer.java +++ b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/CollectionLikeSerializer.java @@ -22,6 +22,7 @@ import java.lang.invoke.MethodHandle; import java.lang.reflect.Constructor; import java.util.Collection; +import java.util.HashSet; import org.apache.fory.Fory; import org.apache.fory.annotation.CodegenInvoke; import org.apache.fory.config.Config; @@ -52,6 +53,7 @@ public abstract class CollectionLikeSerializer extends Serializer { private MethodHandle constructor; private int numElements; private final int collectionOwnerBytes; + private final boolean repairsMutableHash; protected final Config config; protected final boolean supportCodegenHook; protected final TypeInfoHolder elementTypeInfoHolder; @@ -94,6 +96,8 @@ protected CollectionLikeSerializer( super(typeResolver.getConfig(), cls, immutable); this.config = typeResolver.getConfig(); this.collectionOwnerBytes = collectionOwnerBytes; + repairsMutableHash = + typeResolver.getConfig().trackingRef() && !immutable && HashSet.class.isAssignableFrom(cls); this.supportCodegenHook = supportCodegenHook; elementTypeInfoHolder = typeResolver.nilTypeInfoHolder(); this.typeResolver = typeResolver; @@ -128,6 +132,12 @@ public final boolean supportCodegenHook() { return supportCodegenHook; } + /** Captures the active-back-reference epoch for mutable hash Set reads. */ + @CodegenInvoke + public final long hashRefEpoch(ReadContext readContext) { + return repairsMutableHash ? readContext.getRefReader().getMaterializingRefEpoch() : -1; + } + /** * Write data except size and elements. * @@ -447,12 +457,16 @@ private void writeDifferentTypeElements( @Override public T read(ReadContext readContext) { + long hashRefEpoch = hashRefEpoch(readContext); Collection collection = newCollection(readContext); int numElements = getAndClearNumElements(); if (numElements != 0) { readElements(readContext, collection, numElements); } - return onCollectionRead(collection); + if (hashRefEpoch < 0) { + return onCollectionRead(collection); + } + return onCollectionRead(readContext, collection, hashRefEpoch); } /** @@ -597,6 +611,18 @@ private void throwInvalidCollectionSize(int numElements) { public abstract T onCollectionRead(Collection collection); + /** Completes a generated or interpreted collection read using the same hash repair owner. */ + @CodegenInvoke + public final T onCollectionRead( + ReadContext readContext, Collection collection, long hashRefEpoch) { + T value = onCollectionRead(collection); + if (hashRefEpoch >= 0 + && hashRefEpoch != readContext.getRefReader().getMaterializingRefEpoch()) { + HashContainerReadState.trackCollection(readContext, collection); + } + return value; + } + protected void readElements(ReadContext readContext, Collection collection, int numElements) { MemoryBuffer buffer = readContext.getBuffer(); int flags = buffer.readByte(); diff --git a/java/fory-core/src/main/java/org/apache/fory/serializer/collection/HashContainerReadState.java b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/HashContainerReadState.java new file mode 100644 index 0000000000..f49caa0fc8 --- /dev/null +++ b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/HashContainerReadState.java @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.serializer.collection; + +import java.util.Collection; +import java.util.IdentityHashMap; +import java.util.Map; +import java.util.Map.Entry; +import org.apache.fory.collection.ObjectArray; +import org.apache.fory.context.ReadContext; +import org.apache.fory.context.RefReader; + +/** Operation-local repair state for hash containers which observed an incomplete back-reference. */ +final class HashContainerReadState implements RefReader.RefReadListener { + private static final Object CONTEXT_KEY = new Object(); + private static final Object PRESENT = new Object(); + + private final IdentityHashMap seen = new IdentityHashMap<>(); + private final ObjectArray containers = new ObjectArray<>(2); + + static void trackCollection(ReadContext readContext, Collection collection) { + track(readContext, collection); + } + + static void trackMap(ReadContext readContext, Map map) { + track(readContext, map); + } + + private static void track(ReadContext readContext, Object container) { + RefReader refReader = readContext.getRefReader(); + if (!refReader.hasMaterializingRefs()) { + rebuild(container); + return; + } + HashContainerReadState state = + (HashContainerReadState) readContext.getContextObject(CONTEXT_KEY); + if (state == null) { + state = new HashContainerReadState(); + readContext.putContextObject(CONTEXT_KEY, state); + refReader.setRefReadListener(state); + } + if (state.seen.put(container, PRESENT) == null) { + state.containers.add(container); + } + } + + @Override + public void onRefReadsComplete() { + ObjectArray containers = this.containers; + try { + // Inner containers finish and register before their outer owners. Rebuild in that same order + // so an outer key whose hashCode consults an inner container observes repaired lookups. + for (int i = 0; i < containers.size; i++) { + rebuild(containers.objects[i]); + } + } finally { + containers.clear(); + seen.clear(); + } + } + + private static void rebuild(Object container) { + if (container instanceof Map) { + rebuildMap((Map) container); + } else { + rebuildCollection((Collection) container); + } + } + + private static void rebuildCollection(Collection collection) { + // A back-reference must be published before its fields are complete to preserve identity. + // Reordering fields only hides mutable-hash failures, so rebuild the affected buckets after + // materialization while retaining the original container and encounter order. + Object[] elements = collection.toArray(); + collection.clear(); + for (Object element : elements) { + collection.add(element); + } + } + + private static void rebuildMap(Map map) { + Object[] entries = new Object[Math.multiplyExact(map.size(), 2)]; + int index = 0; + for (Object entryObject : map.entrySet()) { + Entry entry = (Entry) entryObject; + entries[index++] = entry.getKey(); + entries[index++] = entry.getValue(); + } + map.clear(); + for (int i = 0; i < index; i += 2) { + map.put(entries[i], entries[i + 1]); + } + } +} diff --git a/java/fory-core/src/main/java/org/apache/fory/serializer/collection/MapLikeSerializer.java b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/MapLikeSerializer.java index 87c327dde6..99b99bba38 100644 --- a/java/fory-core/src/main/java/org/apache/fory/serializer/collection/MapLikeSerializer.java +++ b/java/fory-core/src/main/java/org/apache/fory/serializer/collection/MapLikeSerializer.java @@ -33,6 +33,7 @@ import java.lang.invoke.MethodHandle; import java.lang.reflect.Constructor; +import java.util.HashMap; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; @@ -78,6 +79,7 @@ private MapTypeCache(TypeResolver typeResolver) { protected MethodHandle constructor; protected final Config config; private final int mapOwnerBytes; + private final boolean repairsMutableHash; protected final boolean supportCodegenHook; private final GenericType objType; // For subclass whose kv type are instantiated already, such as @@ -120,6 +122,8 @@ protected MapLikeSerializer( super(typeResolver.getConfig(), cls, immutable); this.config = typeResolver.getConfig(); this.mapOwnerBytes = mapOwnerBytes; + repairsMutableHash = + typeResolver.getConfig().trackingRef() && !immutable && HashMap.class.isAssignableFrom(cls); this.typeResolver = typeResolver; trackRef = typeResolver.getConfig().trackingRef(); this.supportCodegenHook = supportCodegenHook; @@ -589,10 +593,20 @@ protected void copyEntry(CopyContext copyContext, Map originMap, Ob @Override public T read(ReadContext readContext) { + long hashRefEpoch = hashRefEpoch(readContext); Map map = newMap(readContext); int size = getAndClearNumElements(); readElements(readContext, size, map); - return onMapRead(map); + if (hashRefEpoch < 0) { + return onMapRead(map); + } + return onMapRead(readContext, map, hashRefEpoch); + } + + /** Captures the active-back-reference epoch for mutable hash Map reads. */ + @CodegenInvoke + public final long hashRefEpoch(ReadContext readContext) { + return repairsMutableHash ? readContext.getRefReader().getMaterializingRefEpoch() : -1; } public void readElements(ReadContext readContext, int size, Map map) { @@ -1012,4 +1026,15 @@ private void throwInvalidMapBodySize(int numElements) { public abstract T onMapCopy(Map map); public abstract T onMapRead(Map map); + + /** Completes a generated or interpreted map read using the same hash repair owner. */ + @CodegenInvoke + public final T onMapRead(ReadContext readContext, Map map, long hashRefEpoch) { + T value = onMapRead(map); + if (hashRefEpoch >= 0 + && hashRefEpoch != readContext.getRefReader().getMaterializingRefEpoch()) { + HashContainerReadState.trackMap(readContext, map); + } + return value; + } } diff --git a/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java b/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java index aa92eb0266..217b084c62 100644 --- a/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java +++ b/java/fory-core/src/test/java/org/apache/fory/codegen/pkgprivate/PackagePrivateCyclicSetChildLossTest.java @@ -20,10 +20,12 @@ package org.apache.fory.codegen.pkgprivate; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertSame; import static org.testng.Assert.assertTrue; import java.io.Serializable; import java.util.ArrayList; +import java.util.Collections; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; @@ -37,24 +39,19 @@ import org.testng.annotations.Test; /** - * Regression test: a wide fan-out (one parent, many children) cyclic graph of package-private - * nodes silently loses entries from a {@code Set} field during - * deserialization - no exception is thrown, so unlike {@link PackagePrivateMapKeyTest} (which - * catches the codegen CompileException for this same field shape) this defect only surfaces as - * missing data. + * Regression test for a package-private cyclic graph whose child inserts an early parent + * back-reference into a {@link LinkedHashSet}. * - *

Reproduced from a production graph with 100+ children under one parent node; the - * child->parent edge (an EnumMap-keyed back-reference) survives the round trip intact, but the - * parent->child edge (a plain HashSet) loses a handful of entries. Both edges are set together, - * atomically, at construction time, so the source object graph itself is never inconsistent - - * the asymmetry is introduced purely by fory's serialize/deserialize round trip. + *

The set still iterates the exact deserialized parent, but lookup fails after the parent's + * hash-relevant fields finish materializing. A wide fan-out makes the resulting asymmetric lookup + * easy to observe without changing the object graph or throwing an exception. */ public class PackagePrivateCyclicSetChildLossTest { private static final int CHILD_COUNT = 200; @Test - public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() { + public void testCyclicSetMembership() { ThreadSafeFory fury = Fory.builder() .withXlang(false) @@ -80,20 +77,28 @@ public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() { FanOutContainer result = (FanOutContainer) fury.deserialize(bytes); FanOutNode resultParent = result.nodes.get(FanOutType.TYPE_A).get("parent"); - assertEquals(resultParent.children.size(), CHILD_COUNT, "parent lost children from its Set field"); + assertEquals( + resultParent.children.size(), CHILD_COUNT, "parent lost children from its Set field"); - // Every child must still be reachable BOTH ways: down (parent.children) and up - // (child.parents), and both directions must point at the exact same object (fory's - // refTracking should unify identical (type,id) instances, not duplicate them). + // Inspect the child set by iteration before using contains so the test distinguishes stale + // hash buckets from a lost reference-table edge. List asymmetric = new ArrayList<>(); for (String childId : expectedChildIds) { FanOutNode resultChild = result.nodes.get(FanOutType.TYPE_B).get(childId); boolean parentListsChild = resultParent.children.contains(resultChild); - boolean childListsParent = - resultChild.parents.getOrDefault(FanOutType.TYPE_A, Set.of()).contains(resultParent); + Set resultParents = + resultChild.parents.getOrDefault(FanOutType.TYPE_A, Collections.emptySet()); + assertEquals(resultParents.size(), 1); + assertSame(resultParents.iterator().next(), resultParent); + boolean childListsParent = resultParents.contains(resultParent); if (!parentListsChild || !childListsParent) { asymmetric.add( - childId + " (parentListsChild=" + parentListsChild + ", childListsParent=" + childListsParent + ")"); + childId + + " (parentListsChild=" + + parentListsChild + + ", childListsParent=" + + childListsParent + + ")"); } } assertTrue( @@ -102,7 +107,7 @@ public void testWideFanOutDoesNotLoseChildrenFromPackagePrivateSet() { } } -// All package-private — this triggers the bug +// Package-private model retained from the original reproduction. enum FanOutType implements Serializable { TYPE_A, TYPE_B diff --git a/java/fory-core/src/test/java/org/apache/fory/serializer/collection/CyclicHashContainerTest.java b/java/fory-core/src/test/java/org/apache/fory/serializer/collection/CyclicHashContainerTest.java new file mode 100644 index 0000000000..96b11a9b84 --- /dev/null +++ b/java/fory-core/src/test/java/org/apache/fory/serializer/collection/CyclicHashContainerTest.java @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fory.serializer.collection; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertSame; +import static org.testng.Assert.assertThrows; +import static org.testng.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import org.apache.fory.Fory; +import org.apache.fory.builder.Generated; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +public class CyclicHashContainerTest { + @DataProvider + public static Object[][] configs() { + return new Object[][] { + {false, false}, {false, true}, {true, false}, {true, true}, + }; + } + + @Test(dataProvider = "configs") + public void testMutableHashBackrefs(boolean codegen, boolean linked) { + Fory fory = + Fory.builder() + .withXlang(false) + .requireClassRegistration(false) + .withRefTracking(true) + .withCodegen(codegen) + .withAsyncCompilation(false) + .withCompatible(false) + .build(); + + HashNode parent = newGraph(linked); + HashNode result = (HashNode) fory.deserialize(fory.serialize(parent)); + if (codegen) { + assertTrue(fory.getTypeResolver().getSerializer(HashNode.class) instanceof Generated); + } + assertGraph(result, linked); + } + + @Test + public void testStateResetAfterFailure() { + Fory fory = + Fory.builder() + .withXlang(false) + .requireClassRegistration(false) + .withRefTracking(true) + .withCodegen(false) + .withCompatible(false) + .build(); + byte[] bytes = fory.serialize(newGraph(true)); + byte[] truncated = Arrays.copyOf(bytes, bytes.length - 1); + assertThrows(RuntimeException.class, () -> fory.deserialize(truncated)); + assertGraph((HashNode) fory.deserialize(bytes), true); + } + + private static HashNode newGraph(boolean linked) { + HashNode parent = new HashNode("parent"); + HashNode child = new HashNode("child"); + parent.children.add(child); + child.parents = linked ? new LinkedHashSet<>() : new HashSet<>(); + child.parentMap = linked ? new LinkedHashMap<>() : new HashMap<>(); + addBackref(child, new HashNode("before")); + addBackref(child, parent); + addBackref(child, new HashNode("after")); + return parent; + } + + private static void assertGraph(HashNode result, boolean linked) { + HashNode resultChild = result.children.get(0); + assertEquals(resultChild.parents.size(), 3); + assertEquals(resultChild.parentMap.size(), 3); + assertSame(find(resultChild.parents, "parent"), result); + assertSame(find(resultChild.parentMap.keySet(), "parent"), result); + assertTrue(resultChild.parents.contains(result)); + assertEquals(resultChild.parentMap.get(result), "parent-value"); + if (linked) { + assertEquals(ids(resultChild.parents), Arrays.asList("before", "parent", "after")); + assertEquals(ids(resultChild.parentMap.keySet()), Arrays.asList("before", "parent", "after")); + } + } + + private static void addBackref(HashNode child, HashNode parent) { + child.parents.add(parent); + child.parentMap.put(parent, parent.id + "-value"); + } + + private static HashNode find(Collection nodes, String id) { + for (HashNode node : nodes) { + if (id.equals(node.id)) { + return node; + } + } + return null; + } + + private static List ids(Collection nodes) { + List ids = new ArrayList<>(); + for (HashNode node : nodes) { + ids.add(node.id); + } + return ids; + } + + public static class HashNode { + public List children = new ArrayList<>(); + public String id; + public Map parentMap; + public Set parents; + + public HashNode() {} + + HashNode(String id) { + this.id = id; + } + + @Override + public boolean equals(Object object) { + if (this == object) { + return true; + } + if (!(object instanceof HashNode)) { + return false; + } + HashNode hashNode = (HashNode) object; + return Objects.equals(id, hashNode.id); + } + + @Override + public int hashCode() { + return Objects.hashCode(id); + } + } +}