Skip to content

fix(java): reproduce silent Set child-loss for package-private nodes - #3902

Closed
PiotrDuz wants to merge 2 commits into
apache:mainfrom
PiotrDuz:main
Closed

fix(java): reproduce silent Set child-loss for package-private nodes#3902
PiotrDuz wants to merge 2 commits into
apache:mainfrom
PiotrDuz:main

Conversation

@PiotrDuz

@PiotrDuz PiotrDuz commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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.

Why?

fory had a regression bug of cyclic graph losing edges. Want to pin it so no further regressions are possible.

What does this PR do?

This PR adds a test case that reproduces the regression

Related issues

#3903

AI Contribution Checklist

The test has been generated by AI, but the idea and guidance is human - the problem and its reporduction in version 1.1.0 is tested by hand.

…in cyclic graphs

Wide fan-out (one parent, many children) cyclic graph of package-private
nodes loses entries from a Set<PackagePrivateType> 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.

@wenshao wenshao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Critical] PR adds a test that reproduces an active bug (issue #3903) but contains no production code fix. CI confirms all Java jobs fail (Java CI 8/11/17/21/25/26, Openj9, Windows, Code Style Check). Merging would break main with a permanently failing test. The fix(java): commit type signals a bug fix but delivers only a regression test. Either include the production fix, mark the test @ignore with a link to #3903, or change the commit type to test.

— qwen3.8-max-preview via Qwen Code /review

@chaokunyang
chaokunyang marked this pull request as draft August 1, 2026 12:27
@chaokunyang

Copy link
Copy Markdown
Collaborator

After reviewing this issue, cyclic references should not be used as Set elements or Map keys. Fory must publish an object reference before all of its fields have been fully deserialized in order to resolve cycles. If that partially materialized object is inserted into a hash-based container, hashCode() or equals() may observe incomplete state. When the remaining fields are restored, the hash may change and invalidate the container's internal structure.

This happened to work previously because the relevant List or collection field was serialized and deserialized last, after the fields used by hashCode() and equals() had already been restored. That ordering was incidental and is no longer guaranteed after the field-ordering semantics were adjusted.

The recommended workaround is to serialize the cyclic references as a List, mark the derived Set as transient, and materialize the Set lazily after deserialization:

private List<Node> parentList = new ArrayList<>();
private transient Set<Node> parents;

public Set<Node> getParents() {
  if (parents == null) {
    parents = new LinkedHashSet<>(parentList);
  }
  return parents;
}

The same approach can be used for maps by serializing an ordered list of key-value entries and constructing the map after the object graph has been fully deserialized. Lists, arrays, object fields, and map values may still contain cyclic references; the limitation applies specifically to cyclic objects used as set elements or map keys.

@chaokunyang chaokunyang closed this Aug 1, 2026
chaokunyang added a commit that referenced this pull request Aug 1, 2026
## Why?



## What does this PR do?



## Related issues

Closes #3903 #3902

## AI Contribution Checklist



- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence or equivalent persisted links of the
final clean AI review results from both fresh reviewers described in
`AI_POLICY.md`, the Fory-guided reviewer and the independent general
reviewer, on the current PR diff or current HEAD after the latest code
changes.



## Does this PR introduce any user-facing change?



- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Benchmark
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants