CAMEL-20199: Remove ThreadLocal Yaml caching in SnakeYAMLDataFormat - #25240
Conversation
With virtual threads, the ThreadLocal<WeakReference<Yaml>> cache is ineffective because each virtual thread is short-lived and creates a new Yaml instance that is never reused. Since Yaml is not thread-safe (it stores state during parsing/dumping), the simplest correct approach is to create a fresh Yaml instance for each marshal/unmarshal operation. The Yaml constructor cost is modest - the real work happens during parsing, not construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 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).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Looks good ✅
Clean removal of ThreadLocal caching in SnakeYAMLDataFormat in favor of per-operation Yaml instantiation. The rationale is sound: ThreadLocal+WeakReference caching is ineffective with virtual threads (each VT is short-lived, cache never gets a hit), and the Yaml constructor cost is modest relative to parsing/dumping.
Good observations:
- Method rename from
getYaml()tocreateYaml()correctly communicates the new semantics - The concurrent test from CAMEL-24152 (
SnakeYAMLConcurrentTest) provides good coverage for validating this change - Consistent with other CAMEL-20199 ThreadLocal removal work
Minor pre-existing note (not introduced by this PR): when a user provides a custom constructor/representer/dumperOptions via setters, createYaml() reuses that same stateful instance across concurrent calls. Since SnakeYAML's BaseConstructor and Representer are heavily stateful, concurrent use of a shared user-provided instance could be unsafe. The default code path (no user-provided objects) correctly creates fresh instances and is safe. Consider a follow-up ticket to document that user-provided constructor/representer must be thread-safe.
CI passes on both JDK 17 and JDK 25.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Review Summary (informational — not a formal approval)
Clean, well-motivated removal of ineffective ThreadLocal+WeakReference caching in SnakeYAMLDataFormat. With virtual threads the cache miss rate approaches 100%, making per-operation Yaml instantiation the right approach.
Key observations
-
Method rename from
getYaml()tocreateYaml()— Excellent naming choice that correctly communicates each call produces a fresh instance rather than returning a cached one. SincegetYaml()wasprivate, there is no API compatibility concern. -
Thread safety — The default code path (no user-provided constructor/representer) correctly creates fresh instances per call and is safe. The existing
SnakeYAMLConcurrentTest(8 threads × 500 iterations) validates this. -
Pre-existing concern (not introduced by this PR): When a user sets a custom
constructororrepresenter,createYaml()reuses that same stateful instance across concurrent calls. SnakeYAML'sBaseConstructorandRepresentermaintain internal maps during parse/dump, so concurrent use of a shared user-provided instance is unsafe. This was equally unsafe with the ThreadLocal approach, so this PR does not worsen the situation. A follow-up JIRA ticket to document or address this would be valuable. -
Minor:
doStop()now only callssuper.doStop()(a no-op). The override could be removed, but keeping it for symmetry withdoStart()is harmless.
No documentation update needed — purely internal implementation change with no user-visible behavioral difference. Consistent with the broader CAMEL-20199 virtual thread support effort.
Note: This is an informational review only — an agent cannot approve its operator's own PRs. Human review is required.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Clean, well-motivated simplification.
After CAMEL-24152 (fdd9a7ba2b29), the ThreadLocal<WeakReference<Yaml>> cache was already recreating all collaborators (BaseConstructor, Representer, DumperOptions, Resolver) on every call — so the cache only saved the trivial Yaml constructor cost. Removing it:
- Eliminates dead complexity
- Improves virtual thread compatibility (ThreadLocal never hits on short-lived VTs)
getYaml()→createYaml()rename is safe (privatemethod,finalclass)SnakeYAMLConcurrentTest(8 threads × 500 iterations) covers concurrent correctness- CI green on JDK 17 + JDK 25
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
2 participants
Summary
ThreadLocal<WeakReference<Yaml>>caching fromSnakeYAMLDataFormatthat is ineffective with virtual threads (each VT is short-lived, so the cache never gets a hit)Yamlinstance per marshal/unmarshal operation instead (Yaml is not thread-safe, and its constructor cost is modest)getYaml()tocreateYaml()to reflect the new semanticsPart of the broader CAMEL-20199 effort to complete virtual thread support across components.
Test plan
camel-snakeyamlpass (includingSnakeYAMLConcurrentTest)Claude Code on behalf of Guillaume Nodet