Skip to content

bugfix : fastjson2 JSONB concurrent ref deserialization - #8160

Merged
funky-eyes merged 10 commits into
apache:2.xfrom
LegendPei:fix/fastjson2-concurrent-ref
Jul 29, 2026
Merged

bugfix : fastjson2 JSONB concurrent ref deserialization#8160
funky-eyes merged 10 commits into
apache:2.xfrom
LegendPei:fix/fastjson2-concurrent-ref

Conversation

@LegendPei

@LegendPei LegendPei commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Ⅰ. Describe what this PR did

This PR fixes a fastjson2 JSONB deserialization issue where reference-heavy payloads may lose $ref fields under concurrent deserialization.

The affected paths are:

  • seata-serializer-fastjson2
  • Fastjson2UndoLogParser

Both paths enable JSONWriter.Feature.ReferenceDetection and deserialize through JSONB.parseObject(...). This PR synchronizes the JSONB parse section to avoid concurrent fastjson2 reader initialization causing referenced fields to be restored as null.

Ⅱ. Does this pull request fix one issue?

fixes #8159

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@LegendPei
LegendPei marked this pull request as ready for review July 7, 2026 02:51
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.05%. Comparing base (6901934) to head (ec19e3c).

Additional details and impacted files
@@            Coverage Diff            @@
##                2.x    #8160   +/-   ##
=========================================
  Coverage     73.04%   73.05%           
  Complexity     1141     1141           
=========================================
  Files          1151     1152    +1     
  Lines         42275    42288   +13     
  Branches       5045     5046    +1     
=========================================
+ Hits          30881    30892   +11     
+ Misses         8919     8917    -2     
- Partials       2475     2479    +4     
Files with missing lines Coverage Δ
...seata/common/json/Fastjson2ObjectReaderWarmup.java 100.00% <100.00%> (ø)
...datasource/undo/parser/Fastjson2UndoLogParser.java 88.88% <100.00%> (+1.38%) ⬆️
...eata.serializer.fastjson2/Fastjson2Serializer.java 100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a concurrency-related correctness issue in fastjson2 JSONB deserialization where $ref-based references can be lost (restored as null) under concurrent parsing when reader caches are cold/cleared. It does so by serializing JSONB parsing in the two affected deserialization entry points and adds targeted regression/stress tests.

Changes:

  • Serialize JSONB parsing in Fastjson2Serializer.deserialize(...) to avoid concurrent reader initialization issues.
  • Serialize JSONB parsing in Fastjson2UndoLogParser.decode(...) for the same class of issue in undo-log decoding.
  • Add regression + opt-in concurrent stress tests in both affected modules to detect $ref fields being dropped.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
serializer/seata-serializer-fastjson2/src/main/java/org.apache.seata.serializer.fastjson2/Fastjson2Serializer.java Adds a global parse lock around JSONB deserialization to avoid concurrent $ref loss.
rm-datasource/src/main/java/org/apache/seata/rm/datasource/undo/parser/Fastjson2UndoLogParser.java Adds a global parse lock around undo-log JSONB decode to avoid concurrent $ref loss.
serializer/seata-serializer-fastjson2/src/test/java/org/apache/seata/serializer/fastjson2/Fastjson2ConcurrentRefDeserializationTest.java Adds regression + opt-in concurrent stress test for protocol message JSONB reference restoration.
rm-datasource/src/test/java/org/apache/seata/rm/datasource/undo/parser/Fastjson2ConcurrentRefDeserializationTest.java Adds regression + opt-in concurrent stress test for undo-log JSONB reference restoration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@funky-eyes funky-eyes added this to the 2.8.0 milestone Jul 13, 2026
@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. module/rm-datasource rm-datasource module module/serializer serializer module labels Jul 13, 2026
@zrlw

zrlw commented Jul 20, 2026

Copy link
Copy Markdown

The approach that apache/dubbo#16369 used might be more elegant

@slievrly slievrly left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

中文 / Chinese

总体判断

问题真实(fastjson2 的 ObjectReaderProvider 懒初始化在并发下丢 $ref),复现思路(并发线程反序列化引用密集的 payload)和测试写得都很扎实,一眼能看出作者认真调过。

修复方式我不太建议直接合并:加锁的层次和范围偏重,会引入非必要的持续性能损失。

主要问题

1. 加全局 ReentrantLock热路径

Fastjson2Serializer#deserialize 是每一条进入 TC/RM 的消息都会走的路径。把整个反序列化串行到一把 JVM 级别的锁上,等于把 fastjson2 序列化器的吞吐从"多核并发"降到"单线程"。高 QPS 场景下这不是一个可以忽略的常量开销。

Fastjson2UndoLogParser#decode 同理,虽然 undo log 反序列化频率略低,但同样是数据面的东西,性能敏感。

2. Bug 是懒初始化的一次性 race,用永久锁不匹配

按 PR 描述,问题在于"fastjson2 在并发下第一次初始化 reader 时丢字段"。这是首次初始化窗口的问题,一旦 reader cache 建好,后续并发就不再有 race。

更贴合问题的方案:

  • 启动时预热:Seata 初始化阶段做一次单线程 JSONB.parseObject(...)(用几个代表性的类,比如 BranchUndoLogMergedWarpMessage),把 reader cache 填好;之后所有并发路径不加锁。
  • 有 flag 的 DCLAtomicBoolean warmed = new AtomicBoolean(false),只在 warmed=false 时上锁;warm 一次后彻底跳过锁。
  • 推 fastjson2 上游:确认最新 fastjson2 是否已经修,如果修了就升版本;如果没修给 fastjson2 提 issue,比在 Seata 里靠锁绕过更根治。

3. 测试里靠反射清 fastjson2 内部 cache —— 太脆

private static void clearObjectReaderCache() throws Exception {
    for (String fieldName : new String[] {"cache", "cacheFieldBased"}) {
        Field field = ObjectReaderProvider.class.getDeclaredField(fieldName);
        ...
    }
    try {
        Field readerCacheField = ObjectReaderProvider.class.getDeclaredField("readerCache");
        ...
    } catch (NoSuchFieldException ignored) {
        // fastjson2 versions differ in internal cache fields.
    }
}

反射内部字段名 + catch NoSuchFieldException 静默跳过,等于测试的有效性依赖 fastjson2 的具体版本。fastjson2 微升一版把 cache 改名后,测试变成"永远绿"但不再复现 race,回归无从谈起。

建议:

  • 让测试用 -Djava.lang.instrument / 独立 classloader 的方式给每轮跑一个干净 JVM 环境,或
  • 让 fastjson2 通过公开 API 暴露 clear 能力(如果没有就提 PR),或
  • 明确写文档告诉这个测试只对特定 fastjson2 版本区间有意义

4. Fastjson2UndoLogParser.PARSE_LOCKFastjson2Serializer.PARSE_LOCK 是两把独立的锁

如果 fastjson2 的 provider cache 是全局静态的,那两个不同 caller 并发跑时其实还是可能撞在同一个内部初始化 race 上。这两把锁的隔离范围是不够全面的。要么用一把全局锁(更慢但正确),要么按上面 §2 的思路彻底避免锁。

建议

  • **优先走"预热 + 无锁"**方案。启动时做一遍 warmup 就能把懒初始化的 race 消掉,运行时无额外开销。
  • 如果坚持锁方案,至少放一个 AtomicBoolean warmed 短路,几秒后就走无锁路径。
  • 测试改为可复现的、不依赖 fastjson2 内部字段名的版本。
  • 同时给 fastjson2 上游报 issue(如果还没报),根治比在下游绕过更好。

English

Overall

The bug is real (fastjson2's ObjectReaderProvider lazy-init race drops $ref fields under concurrent access). The repro (many threads decoding a reference-heavy payload) and tests are well-thought-out — clear evidence the author actually reproduced it.

But I'd push back on this specific fix: a JVM-wide ReentrantLock around every deserialize call is a heavy, permanent cost for a one-shot init-time bug.

Main concerns

1. Global lock on a hot path

Fastjson2Serializer#deserialize runs on every inbound message on TC/RM. Serializing every call behind one JVM-wide lock turns fastjson2 deserialization from "multi-core parallel" into "single-threaded". Under load this is a real, sustained throughput regression, not a negligible constant.

Fastjson2UndoLogParser#decode has the same shape — lower QPS but still on the data path.

2. The bug is a one-off init-time race; a permanent lock is a mismatch

Per the PR description, fastjson2 loses fields "when initializing readers concurrently for the first time". Once the reader cache is warmed, subsequent concurrent decodes are safe. A better-fitting fix:

  • Startup warmup: at Seata init, run one single-threaded JSONB.parseObject(...) per representative type (BranchUndoLog, MergedWarpMessage, …). Cache is populated; all subsequent concurrent decodes go lock-free.
  • DCL with a flag: AtomicBoolean warmed — take the lock only until warmed flips to true, then no-op the lock path.
  • Push it upstream: check whether the latest fastjson2 has a fix; if so, bump. If not, open an issue upstream — fixing the root cause is cleaner than working around it downstream.

3. Tests clear fastjson2's internal caches by reflection — fragile

private static void clearObjectReaderCache() throws Exception {
    for (String fieldName : new String[] {"cache", "cacheFieldBased"}) {
        Field field = ObjectReaderProvider.class.getDeclaredField(fieldName);
        ...
    }
    try {
        Field readerCacheField = ObjectReaderProvider.class.getDeclaredField("readerCache");
        ...
    } catch (NoSuchFieldException ignored) {
        // fastjson2 versions differ in internal cache fields.
    }
}

Reflecting on internal fields and silently swallowing NoSuchFieldException means the test's effectiveness depends on the specific fastjson2 version. When fastjson2 renames cache in a point release, this test becomes "always green" — it no longer reproduces the race, so any future regression slips through.

Options:

  • Run each round in a fresh classloader / JVM to get a clean provider,
  • Push fastjson2 to expose a public clear API (open a PR upstream), or
  • Explicitly document that this test is meaningful only within a specific fastjson2 version range.

4. Fastjson2UndoLogParser.PARSE_LOCK and Fastjson2Serializer.PARSE_LOCK are two separate locks

If fastjson2's provider cache is a JVM-global static, two different callers concurrently exercising it can still trip the internal init race — the two locks don't overlap. Either use a single shared lock (slower but correct), or better, remove the locks entirely per §2.

Suggested direction

  • Prefer startup-warmup + lock-free at runtime. One JSONB.parseObject per representative type at init time eliminates the race without ongoing cost.
  • If a lock is really necessary, at least short-circuit past it with an AtomicBoolean warmed after a few seconds so the steady state is lock-free.
  • Rewrite the test to not depend on fastjson2's internal field names.
  • File an upstream fastjson2 issue in parallel — a root-cause fix beats a downstream workaround.

@funky-eyes funky-eyes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@funky-eyes
funky-eyes merged commit 5ba6a62 into apache:2.x Jul 29, 2026
13 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module/rm-datasource rm-datasource module module/serializer serializer module type: bug Category issues or prs related to bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent fastjson2 JSONB deserialization can produce null $ref fields

6 participants