CAMEL-24227: Add volatile to JMX-writable fields read on routing threads#24985
CAMEL-24227: Add volatile to JMX-writable fields read on routing threads#24985gnodet wants to merge 3 commits into
Conversation
Fields writable via JMX @ManagedAttribute setters were stored as plain (non-volatile) fields in 12 engine classes but read on routing threads without JMM visibility guarantees. For simple boolean/int/long fields: added volatile. On x86 this compiles to the same instruction as a plain load, so there is zero performance cost. On ARM/POWER architectures with weaker memory ordering, this provides the required happens-before guarantee. For compound writes (tracePattern+patterns, traceFilter+predicate in BacklogTracer/DefaultTracer): replaced the two separate fields with an immutable holder record swapped atomically via a single volatile reference, ensuring routing threads always see a consistent pair. Affected classes: BacklogTracer, DefaultTracer, BaseProcessorSupport, DefaultBacklogDebugger, TotalRequestsThrottler, AbstractThrottler, DefaultStreamCachingStrategy, ThrottlingInflightRoutePolicy, ThrottlingExceptionRoutePolicy, ManagedPerformanceCounter, ScheduledPollConsumer, Delayer. 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:
|
…ompound writes The setMaxInflightExchanges and setResumePercentOfMax setters each write two coupled fields (max + derived resume threshold). Two separate volatile writes don't guarantee the routing thread sees them atomically — it could observe the new maxInflightExchanges with the stale resumeInflightExchanges. Use an immutable ThrottlingLimits holder record so throttle() snapshots both values in a single volatile read. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Review Summary
This PR addresses a systemic JMM-unsafe pattern across 12 engine classes where JMX-writable fields lacked memory visibility guarantees for routing threads. The fix is methodical and well-scoped: simple fields get volatile, and compound writes (three cases) are correctly replaced with immutable holder records swapped via a single volatile reference. After thorough analysis of all 12 files and their corresponding MBeans, the changes are correct and complete.
What I Love
-
The holder record pattern is textbook-perfect. In all three compound-write cases (
BacklogTracer.TracePatternHolder/TraceFilterHolder,DefaultTracer.TracePatternHolder,ThrottlingInflightRoutePolicy.ThrottlingLimits), the snapshot-then-use idiom reads the volatile reference exactly once into a local, guaranteeing routing threads never see a torn pair. The method signatures were also updated to pass the snapshot values as parameters (e.g.,shouldTracePattern(definition, ph.patterns()),startConsumer(size, consumer, resumeInflight)) rather than re-reading the holder -- this is the right way to propagate a consistent snapshot through a call chain. -
The PR silently fixes a pre-existing bug in
BacklogTracer.setTraceFilter(null). The old code settraceFilter = nullbut never clearedpredicate, leavingshouldTraceFilter()evaluating a stale predicate after the filter was removed. The new code correctly setstraceFilterHolder = null, and theif (fh != null)guard inshouldTrace()skips the filter check entirely. The same fix applies tosetTracePattern(null). -
Discipline in scoping. Fields only set during construction or lifecycle startup (e.g.,
AbstractThrottler.rejectExecution,ScheduledPollConsumer.initialDelay,ScheduledPollConsumer.timeUnit,DefaultBacklogDebugger.suspendMode) are correctly left non-volatile. The PR doesn't over-volatilize, which shows careful analysis of which fields are truly written by JMX at runtime vs. configured once before routing threads start.
Findings
Critical
None -- nice work!
Important
None.
Suggestions & Nits
[Question] DefaultTracer.traceCounter (line 62): This is a plain long that's incremented with traceCounter++ on routing threads and read/reset via JMX (ManagedTracer.getTraceCounter()/resetTraceCounter()). Strictly speaking, this has two JMM issues: (a) visibility -- JMX reads may see stale values, and (b) atomicity -- the ++ is a non-atomic read-modify-write. BacklogTracer already uses AtomicLong for the same purpose. I understand this is the reverse direction (routing writes -> JMX reads, vs. this PR's focus on JMX writes -> routing reads), so it's out of scope. Would it be worth a follow-up ticket to align DefaultTracer.traceCounter with BacklogTracer's AtomicLong pattern?
Overall
Clean, well-reasoned concurrency fix. The approach of volatile for simple fields + immutable holders for compound state is the standard JMM-safe pattern, and it's applied consistently across all 12 classes. On x86 the volatile reads compile to plain loads (zero overhead); on ARM/POWER the memory barriers are exactly what's needed. The bonus fix for the stale-predicate bug in setTraceFilter(null) is a nice win. LGTM.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 550 tested, 29 compile-only — current: 550 all testedMaveniverse Scalpel detected 579 affected modules (current approach: 550).
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Went through each field's read and write sites rather than just checking the modifier. The core of this is right: for every field the PR marks volatile, the reader side does a plain read with no compound action, so visibility is the correct and sufficient fix. The three genuine read-modify-write cases were correctly spotted and moved to immutable holders instead — that's the right distinction to draw, and it's the part these PRs usually get wrong.
Two things I'd like to see before this lands, plus one unrelated bug I tripped over.
The sweep is incomplete for the disabled flag
BaseProcessorSupport.disabled is now volatile, but two siblings with the identical writer/reader pair were left plain:
core/camel-core-processor/src/main/java/org/apache/camel/processor/BaseDelegateProcessorSupport.java:31—private boolean disabled;core/camel-core-processor/src/main/java/org/apache/camel/processor/WrapProcessor.java:32—private boolean disabled;
Both are written from JMX via ManagedProcessor.enable() / disable() (core/camel-management/src/main/java/org/apache/camel/management/mbean/ManagedProcessor.java:232, :239 → da.setDisabled(...)) and read on the routing hot path at core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/CamelInternalProcessor.java:305 (... instanceof DisabledAware da && da.isDisabled()).
That matters because BaseDelegateProcessorSupport is the base of FilterProcessor, LoopProcessor, CatchProcessor, FinallyProcessor, DelayProcessorSupport (hence Delayer, which this PR does touch), SagaProcessor and others, while WrapProcessor is installed by the policy/transacted reifiers. So as it stands, enable()/disable() over JMX remains unreliable for the majority of processors it applies to — which is the exact bug this PR sets out to fix.
DefaultStreamCachingStrategy.UtilizationStatistics.statisticsEnabled looks like the same category and is in a file you're already editing: written from ManagedStreamCachingStrategy.setStatisticsEnabled, read on the caching path in doCache. Same pattern as ManagedPerformanceCounter, which you did make volatile.
The setTraceFilter(null) fix needs a test
BacklogTracer.setTraceFilter(null) previously nulled traceFilter but left predicate at its old value, so shouldTrace() kept applying a filter the user had just cleared. Nulling the holder fixes it, and it's reachable over JMX through ManagedBacklogTracer. That's a genuine, deterministic bug fix rather than a visibility change — the project rule is that bug fixes come with a test, and this one is a plain unit test. Worth an upgrade-guide line too, since the observable behaviour changes.
Correcting something I initially thought was a blocker
I want to flag this explicitly because it nearly went in as an API-break finding: shouldTracePattern in DefaultTracer gains a String[] patterns parameter, and DefaultTracer is public and non-final — but the method is private, not protected. So there is no source or binary break and no upgrade-guide obligation. Disregard if anyone raises it.
Smaller points
ScheduledPollConsumer.delay:volatilehere has no practical effect. It's snapshot into the scheduler inafterConfigured-style setup (scheduler.setDelay(delay)and friends), not read fromrun()as the PR body's table suggests. More to the point,initialDelay,timeUnitanduseFixedDelayare equally JMX-writable and read in the same place but were left plain — so the treatment is inconsistent either way. Suggest doing all four or none.ThrottlingInflightRoutePolicy.setResumePercentOfMaxstill does a read-modify-write across the pair (it readsmaxInflightExchangesto compute the resume value), so two concurrent JMX writers can still publish a holder built from a mixed snapshot. The holder fixes reader-side tearing, not writer-side atomicity.resumeInflightExchangesis now write-only —startConsumertakes it as a parameter, there's no getter, andtoString()doesn't use it. The default is also duplicated between the field initialisers and the literalnew ThrottlingLimits(...).ThrottlingInflightRoutePolicyhas no@ManagedAttributeat all — it's a@Metadata(label = "bean")RoutePolicy. The "JMX-writable" framing doesn't apply to that class, though the visibility argument still holds for programmatic reconfiguration.- Reverse-direction gaps (you flagged the first yourself):
DefaultTracer.traceCounteris a plainlongincremented non-atomically on routing threads and read/reset over JMX, andDelayer.delayValuelikewise. Both are non-atomic on 32-bit JVMs. Worth a follow-up ticket rather than growing this PR.
Unrelated bug in a file this PR touches
While tracing statisticsEnabled I found a live defect in DefaultStreamCachingStrategy on main — not introduced here, and not something I'd ask you to fix in this PR:
void updateSpool(long size) {
lock.lock();
try {
spoolAverageSize.set(spoolSize.addAndGet(size) / spoolCounter.incrementAndGet());
} finally {
lock.lock(); // should be unlock()
}
}updateMemory directly above it correctly calls unlock(). Because lock is a ReentrantLock, the calling thread doesn't self-deadlock — it just leaves the hold count at 2, permanently — but any other thread entering updateSpool blocks forever. It's reachable whenever stream-caching statistics are enabled and spooling to disk occurs. I've raised it separately so it doesn't get lost in this review.
CI is green. Nothing here is blocking from my side; the incomplete sweep is the one I'd most like addressed, since a partial fix here is easy to mistake for a complete one later.
Reviewed with Claude Code (Opus 4.8) on behalf of Andrea Cosentino (@oscerd). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…-fiel Resolve merge conflict in BacklogTracer.java: keep volatile on all JMX-writable fields (confirmed via ManagedBacklogTracerMBean interface) and update the comment to accurately reflect which fields need volatile. CAMEL-24229 on main made enabled/standby/activityEnabled volatile but missed the other JMX-writable fields (removeOnDump, bodyMaxChars, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Review Summary
The core change is correct and valuable — volatile for simple fields, immutable holders for compound writes is textbook JMM-safe. CI is green and the scope is well-defined.
I want to flag one factual correction on oscerd's review and echo two of their actionable findings.
Factual correction: DefaultTracer.shouldTracePattern IS protected, not private
oscerd's review dismissed the signature change saying "the method is private, not protected." This is incorrect — on main at line 350 the method is protected boolean shouldTracePattern(NamedNode definition). The PR changes it to protected boolean shouldTracePattern(NamedNode definition, String[] patterns). Since DefaultTracer is a public non-final class, this is technically a source/binary compatibility break for anyone extending it and overriding the method. There are no known subclasses in the codebase, and the class lives in camel-base-engine (internal), so the practical risk is very low — but the concern was incorrectly dismissed.
Confirming: incomplete disabled sweep
oscerd correctly flagged that BaseDelegateProcessorSupport.disabled and WrapProcessor.disabled have the identical JMX-write / routing-read pattern as BaseProcessorSupport.disabled but were left plain. Since BaseDelegateProcessorSupport is the base of FilterProcessor, LoopProcessor, CatchProcessor, FinallyProcessor, DelayProcessorSupport and others, the enable()/disable() JMX operation remains unreliable for the majority of processors. Should be addressed in this PR or an explicit follow-up.
Confirming: setTraceFilter(null) bugfix deserves a test
The old code set traceFilter = null but never cleared predicate, leaving shouldTrace() evaluating a stale predicate. The new holder pattern correctly fixes this. This is a deterministic behavior change that warrants a test per project conventions.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
|
|
||
| protected boolean shouldTracePattern(NamedNode definition) { | ||
| protected boolean shouldTracePattern(NamedNode definition, String[] patterns) { |
There was a problem hiding this comment.
This is a protected method in a public non-final class — oscerd's review dismissed this as private, but it is protected on main (line 350). The signature change from shouldTracePattern(NamedNode) to shouldTracePattern(NamedNode, String[]) is a source/binary compatibility break for any downstream code extending DefaultTracer and overriding this method.
No known subclasses exist in the Camel codebase, and the class is in camel-base-engine (internal module), so practical risk is very low. Just flagging that the concern was dismissed on incorrect grounds.
| this.traceFilterHolder = new TraceFilterHolder(filter, p); | ||
| } else { | ||
| this.traceFilterHolder = null; |
There was a problem hiding this comment.
This else branch fixes a pre-existing bug: the old code set traceFilter = null but never cleared predicate, so shouldTrace() would keep evaluating a stale predicate after the user cleared the filter via JMX. The fix is correct.
Since this is a deterministic behavior change (not just a visibility fix), it should have a test — e.g. set a filter, verify it's applied, clear it with setTraceFilter(null), verify filtering is disabled.
3 participants
Claude Code on behalf of gnodet
Summary
An audit of the Camel management layer revealed a systemic JMM-unsafe pattern across 12 engine classes (~50 fields). Fields writable via JMX
@ManagedAttributesetters are stored as plain (non-volatile) fields in engine classes, but read on routing threads without any memory visibility guarantee.Fix approach
Simple boolean/int/long fields: added
volatile. On x86 this compiles to the same instruction as a plain load, so there is zero performance cost. On ARM/POWER architectures with weaker memory ordering, this provides the required happens-before guarantee.Compound writes (three cases): replaced coupled field pairs with immutable holder records swapped atomically via a single volatile reference, ensuring routing threads always see consistent values:
BacklogTracer:tracePattern+patterns→TracePatternHolder,traceFilter+predicate→TraceFilterHolderDefaultTracer:tracePattern+patterns→TracePatternHolderThrottlingInflightRoutePolicy:maxInflightExchanges+resumeInflightExchanges→ThrottlingLimits(thesetMaxInflightExchanges/setResumePercentOfMaxsetters each compute a derivedresumeInflightExchanges— two separate volatile writes don't guarantee the routing thread sees both atomically)Affected classes (12 files)
BacklogTracerenabled,standby,bodyMaxChars,bodyIncludeStreams,bodyIncludeFiles,includeExchangeProperties,includeExchangeVariables,includeException,backlogSize,activitySize,removeOnDump,activityEnabled,traceRests,traceTemplates+ holder records fortracePattern+patternsandtraceFilter+predicateshouldTrace(),traceNode()DefaultTracerenabled,standby,traceRests,traceTemplates,traceBeforeAndAfterRoute+ holder record fortracePattern+patternsshouldTrace()BaseProcessorSupportdisabledCamelInternalProcessor.process()DefaultBacklogDebuggerfallbackTimeout,bodyMaxChars,bodyIncludeStreams,bodyIncludeFiles,includeExchangeProperties,includeExchangeVariables,includeException,singleStepIncludeStartEndNodeBreakpoint.beforeProcess()TotalRequestsThrottlertimePeriodMillisprocess()AbstractThrottlermaxRequestsExpressionprocess()DefaultStreamCachingStrategyspoolThreshold,spoolUsedHeapMemoryThreshold,anySpoolRules,bufferSizeshouldSpoolCache()ThrottlingInflightRoutePolicymaxInflightExchanges,resumePercentOfMax,resumeInflightExchanges,scope+ holder record formaxInflightExchanges+resumeInflightExchangesthrottle()ThrottlingExceptionRoutePolicyhalfOpenAfter,failureWindow,failureThresholdcalculateState()ManagedPerformanceCounterstatisticsEnabledDefaultInstrumentationProcessor.before()ScheduledPollConsumergreedy,sendEmptyMessageWhenIdle,delay,runLoggingLeveldoRun()DelayerdelaycalculateDelay()Test plan
formatter:format+impsort:sortreports no changes)Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com