CAMEL-24152: Fix concurrent marshal/unmarshal data corruption in camel-snakeyaml#24814
Conversation
…l-snakeyaml Since CAMEL-22354 (4.15.0), all per-thread Yaml instances shared single BaseConstructor and Representer singletons. These SnakeYAML classes are heavily stateful and mutated on every load/dump, causing concurrent crashes and silent cross-thread data bleed. Create fresh BaseConstructor, Representer, DumperOptions, and Resolver per cached Yaml in getYaml(), restoring per-thread construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
LGTM — important concurrency fix for a real data-corruption bug.
Root cause: CAMEL-22354 (5dfc5c2857e9, Aug 2025) replaced per-thread Function<CamelContext, BaseConstructor> factories with shared singletons. The ThreadLocal<Yaml> cache remained, but all per-thread Yaml instances now shared the same BaseConstructor, Representer, DumperOptions, and Resolver. Since SnakeYAML's BaseConstructor and Representer mutate internal state on every load()/dump(), concurrent exchanges produce crashes (IndexOutOfBoundsException, NPEs) and silent cross-thread data corruption.
The fix: Correct and minimal — moves construction of default BaseConstructor, Representer, DumperOptions, and Resolver into getYaml() so each thread-cached Yaml gets fresh instances via the default*() factory methods. User-supplied custom instances (via setters) are still used as-is, which is a reasonable trade-off — restoring the factory pattern for custom instances would require an API change.
The test: Good stress test — 8 threads × 500 marshal/unmarshal round-trips with distinct per-thread payloads, CountDownLatch for maximum contention, and explicit per-iteration data integrity assertion.
Minor note: The dual LoaderOptions pattern (one in defaultConstructor(), another in getYaml()) is pre-existing — the PR preserves the original behavior.
This should be considered for backport to 4.18.x if 4.15.0+ releases are affected.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
LGTM — correct thread-safety fix for a real data-corruption bug. The shared SnakeYAMLDataFormat reused stateful BaseConstructor/Representer/DumperOptions/Resolver singletons across threads (snakeyaml's Yaml isn't thread-safe), so concurrent marshal/unmarshal corrupted output. Building fresh default*() instances per thread-cached Yaml isolates them; since they're cached in the existing per-thread ThreadLocal<WeakReference<Yaml>> (created once on cache-miss, not per call) there's no perf cliff and no new ThreadLocal leak, and doStop() still clears the cache. SnakeYAMLConcurrentTest (8 threads × 500 round-trips) is a good regression guard.
Two non-blocking notes: a user-supplied custom constructor/representer (via the setters) is still shared across threads — opt-in and out of scope here, but worth a follow-up if you want full isolation. And getConstructor()/getRepresenter()/etc. now return null after start when not user-set (was internal defaults) — no signature change and nothing internal depends on it, just flagging the observable getter change.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
4 participants
Summary
Claude Code on behalf of Croway
Fixes CAMEL-24152: concurrent
marshal/unmarshaloperations corrupt data because all per-threadYamlinstances share singleBaseConstructorandRepresentersingletons created once indoStart().This was a regression introduced by CAMEL-22354 (commit
5dfc5c2857e9, released in 4.15.0), which replacedFunction<CamelContext, BaseConstructor>factories with shared singletons while retaining theThreadLocal<Yaml>cache. Since SnakeYAML'sBaseConstructorandRepresenterare heavily stateful, sharing them across threads causes:IndexOutOfBoundsException,ParserException,NPE)NullPointerExceptionon nodes)Changes
SnakeYAMLDataFormat.doStart()— Removed default singleton creation forconstructor,representer,dumperOptions,resolver.SnakeYAMLDataFormat.getYaml()— When creating a newYaml, callsdefault*()factory methods to produce fresh per-thread instances instead of reusing shared singletons. User-supplied custom instances (viasetConstructor()etc.) are still used as-is.SnakeYAMLConcurrentTest— New test: 8 threads × 500 marshal/unmarshal round-trips with distinct payloads, asserting no exceptions and no cross-thread data corruption.Test plan
mvn verifyincomponents/camel-snakeyaml— all 19 tests pass (including new concurrency test)🤖 Generated with Claude Code