bugfix : fastjson2 JSONB concurrent ref deserialization - #8160
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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
$reffields 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.
|
The approach that apache/dubbo#16369 used might be more elegant |
slievrly
left a comment
There was a problem hiding this comment.
中文 / 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(...)(用几个代表性的类,比如BranchUndoLog、MergedWarpMessage),把 reader cache 填好;之后所有并发路径不加锁。 - 有 flag 的 DCL:
AtomicBoolean 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_LOCK 与 Fastjson2Serializer.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 untilwarmedflips totrue, 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
clearAPI (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.parseObjectper 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 warmedafter 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.
Ⅰ. Describe what this PR did
This PR fixes a fastjson2 JSONB deserialization issue where reference-heavy payloads may lose
$reffields under concurrent deserialization.The affected paths are:
seata-serializer-fastjson2Fastjson2UndoLogParserBoth paths enable
JSONWriter.Feature.ReferenceDetectionand deserialize throughJSONB.parseObject(...). This PR synchronizes the JSONB parse section to avoid concurrent fastjson2 reader initialization causing referenced fields to be restored asnull.Ⅱ. 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