Fix fastjson2 concurrent ref deserialization - #16369
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16369 +/- ##
=========================================
Coverage 60.86% 60.87%
- Complexity 11763 11772 +9
=========================================
Files 1953 1953
Lines 89262 89266 +4
Branches 13471 13470 -1
=========================================
+ Hits 54331 54337 +6
- Misses 29329 29349 +20
+ Partials 5602 5580 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR addresses a fastjson2 JSONB concurrency issue where $ref-heavy payloads can deserialize incorrectly under multi-threaded access, leading to lost nested fields (e.g., shared lists becoming null). The fix introduces a synchronized parsing path in FastJson2ObjectInput and adds a regression test to reproduce the concurrent $ref deserialization failure.
Changes:
- Route both
FastJson2ObjectInput#readObjectoverloads through a sharedparseObject(...)helper. - Synchronize fastjson2 JSONB parsing using a per-
Classlock (ClassValue) to prevent concurrent corruption. - Add a concurrency regression test that stresses
$refresolution under high thread counts and cache resets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dubbo-serialization/dubbo-serialization-fastjson2/src/main/java/org/apache/dubbo/common/serialize/fastjson2/FastJson2ObjectInput.java | Adds synchronized parse helper guarded by a ClassValue lock to avoid concurrent $ref corruption. |
| dubbo-serialization/dubbo-serialization-fastjson2/src/test/java/org/apache/dubbo/common/serialize/fastjson2/FastJson2SerializationTest.java | Adds a regression test for concurrent $ref deserialization and cache-clearing repro. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int i = 0; i < threadCount; i++) { | ||
| Thread thread = new Thread(() -> { | ||
| try { | ||
| barrier.await(); | ||
| if (countNullIds(serialization, url, bytes) > 0) { | ||
| roundNullTasks.incrementAndGet(); | ||
| } | ||
| } catch (Throwable throwable) { | ||
| failure.compareAndSet(null, throwable); | ||
| } finally { | ||
| endLatch.countDown(); | ||
| } | ||
| }); | ||
| thread.start(); | ||
| } | ||
| endLatch.await(); | ||
| if (failure.get() != null) { | ||
| throw new AssertionError("Concurrent deserialization failed", failure.get()); | ||
| } |
| for (int i = 0; i < threadCount; i++) { | ||
| Thread thread = new Thread(() -> { | ||
| try { |
| private <T> T parseObject(byte[] bytes, Class<T> cls, Fastjson2SecurityManager.Handler securityFilter) { | ||
| synchronized (getParseLock(cls)) { | ||
| if (securityFilter.isCheckSerializable()) { |
zrlw
left a comment
zrlw
left a comment
There was a problem hiding this comment.
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:
AtomicBoolean warmed — take the lock only until warmed flips to true, then no-op the lock path.
| @SuppressWarnings("unchecked") | ||
| private void clearObjectReaderCache() throws Exception { | ||
| ObjectReaderProvider provider = JSONFactory.getDefaultObjectReaderProvider(); | ||
| for (String fieldName : new String[] {"cache", "cacheFieldBased"}) { |
There was a problem hiding this comment.
Reflecting on internal fields means the test's effectiveness depends on the specific fastjson2 version. When fastjson2 renames cache in a point release, this test will fail.
Options:
Run each round in a fresh classloader / JVM to get a clean provider,
Explicitly document that this test is meaningful only within a specific fastjson2 version range.
What is the purpose of the change?
Fixes #16368.
Fastjson2 JSONB deserialization can lose nested fields when multiple threads deserialize the same
$ref-heavy payload for the same target class at the same time. In the failing case, shared lists referenced by multiple child objects can becomenullafter concurrent deserialization.This change routes both
FastJson2ObjectInput#readObjectoverloads through a shared helper and synchronizes the fastjson2 parse per requested target class. The lock is backed byClassValue, so it does not retain application classes or classloaders, and unrelated DTO classes can still deserialize concurrently.The regression test serializes an object graph with shared nested lists, verifies a single read succeeds, then repeatedly deserializes the same payload from 200 threads while clearing fastjson2 reader caches between rounds. Without the fix, the test reports a non-zero count of null nested fields.
Tests:
FastJson2SerializationTest#testConcurrentReadObjectWithReferencesFastJson2SerializationTest,TypeMatchTestChecklist