CAMEL-24107: camel-xslt - stylesheet reload is not thread-safe, stale pooled transformers keep serving the old stylesheet#24776
Merged
Merged
Conversation
… pooled transformers keep serving the old stylesheet Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
approved these changes
Jul 16, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Thorough concurrency fix — well-analyzed and well-tested.
The four problems identified are all real:
-
volatile template— correct. This field is read byprocess()threads and written bysetTemplate(). Withoutvolatile, JMM allows stale reads. -
Generation-based pool invalidation (
AtomicLong templateGeneration) — elegant approach. Capturing the generation before borrowing and checking on release ensures stale transformers are discarded rather than re-pooled. ThesetTemplate()→clear()combination provides belt-and-suspenders:clear()evicts idle transformers immediately, the generation check catches in-flight ones. -
transformerSourceLockaroundsetTransformerSource()— correct. JAXPTransformerFactorydoes not guarantee thread safety, and this method mutates error listener + URI resolver + callsnewTemplates(). Serializing access is the right call. -
DCL
reloadLockinXsltEndpoint.onExchange()— prevents redundant parallel reloads whencacheClearedis set (JMX operation). Verified thatloadResource()resetscacheCleared = false(line 397) inside the lock, so the inner check correctly prevents duplicate reloads.
Tests are solid — three scenarios covering basic pool invalidation, multiple reloads (back-and-forth), and concurrent processing with 8 threads after reload. The example2.xsl (<farewell>) vs example.xsl (<goodbye>) makes it easy to distinguish which template was used.
Static analysis: ast-grep flagged two pre-existing getParameters() mutable collection return issues — not introduced by this PR.
Checklist:
- ✅ Tests: 3 new tests covering pool invalidation, multi-reload, and concurrency
- ✅ No public API changes (internal
releaseTransformersignature change is private) - ✅ Backward compatible — behavior unchanged for single-threaded / non-reload paths
- ✅ No security concerns
- ⏳ CI: builds in progress
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
4 tasks
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 472 tested, 29 compile-only — current: 472 all testedMaveniverse Scalpel detected 501 affected modules (current approach: 472).
|
oscerd
reviewed
Jul 16, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
Good fix — this is a real improvement over the status quo, where releaseTransformer() unconditionally re-pooled a borrowed Transformer even if it was borrowed before a setTemplate(), so the pool kept serving the old compiled stylesheet indefinitely after a reload. Stamping a borrow-time templateGeneration and discarding on mismatch, making template volatile, and DCL-guarding the reload in XsltEndpoint all look correct, and the latch-based tests avoid Thread.sleep.
One residual window worth a look, since this is squarely a thread-safety fix: setTemplate() does templateGeneration.incrementAndGet() and transformers.clear() as two separate steps, while process() captures gen = templateGeneration.get() before polling. Interleaving: a reload bumps gen 0→1, a concurrent process() reads gen=1, then poll() hands back an old (gen-0) transformer that clear() hasn't removed yet; on release, generation != templateGeneration.get() is 1 != 1 → false, so that stale transformer is re-offered and keeps serving the old stylesheet until the next reload. Narrow (two adjacent statements) and only under contentCache=false high concurrency, but sticky once hit. Stamping each transformer with its own creation generation (and comparing that on release, rather than the borrow-time gen) would close it fully. The current testConcurrentProcessingDuringReload reloads first and then spawns threads, so it exercises reload-then-read rather than reload-concurrent-with-transform — a test on that ordering would guard the window.
Non-blocking — net still much better than main.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Claude Code on behalf of davsclaus
Fixes thread-safety issues in camel-xslt stylesheet reload (
contentCache=false/clearCachedStylesheetJMX operation) where stale pooled transformers keep serving the old stylesheet after a reload.Problems fixed
transformerCacheSize > 0, aTransformerborrowed beforesetTemplate()is re-offered to the pool after the transform completes, causing the old stylesheet to be served indefinitelyXsltBuilder.templatenot volatile — plain field with no visibility guarantee across threadsTransformerFactoryused concurrently —setTransformerSource()mutates error listener/URI resolver and callsnewTemplates()without synchronization; JAXP does not guaranteeTransformerFactorythread safetycacheClearedflag reset only after full load; concurrent messages trigger redundant stylesheet compilationsChanges
XsltBuilder: maketemplatevolatile; addAtomicLong templateGenerationcounter incremented on everysetTemplate()call; inreleaseTransformer(), discard the transformer if its generation doesn't match current (prevents stale re-entry); wrapsetTransformerSource()with aReentrantLockto serializeTransformerFactoryaccessXsltEndpoint: addReentrantLock reloadLockwith double-checked locking aroundloadResource()inonExchange()to prevent redundant parallel reloadsTest plan
XsltBuilderReloadTestwith 3 tests: basic pool invalidation on template change, multiple reloads, concurrent processing after reloadXsltBuilderTest,XsltTest,XsltRouteTest)camel-coretest suite passes🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com