Guillaume Nodet created CAMEL-24227:
---------------------------------------
Summary: Add volatile to JMX-writable fields read on routing
threads
Key: CAMEL-24227
URL: https://issues.apache.org/jira/browse/CAMEL-24227
Project: Camel
Issue Type: Improvement
Components: camel-management
Reporter: Guillaume Nodet
An audit of the Camel management layer revealed a systemic JMM-unsafe pattern
across 12 engine classes (~50 fields). Fields writable via JMX
@ManagedAttribute setters are stored as plain (non-volatile) fields in engine
classes, but read on routing threads without any memory visibility guarantee.
This was discovered while investigating the BacklogTracerActivityTest flaky
test (PR [#24713|https://github.com/apache/camel/pull/24713]), where
activityEnabled, enabled, and standby fields in BacklogTracer were not volatile.
h3. Affected classes
||Engine class||Non-volatile JMX-writable fields||Hot path||
|BacklogTracer|tracePattern+patterns, traceFilter+predicate, bodyMaxChars,
bodyIncludeStreams, bodyIncludeFiles, includeExchangeProperties,
includeExchangeVariables, backlogSize, activitySize,
removeOnDump|shouldTrace(), traceNode()|
|DefaultTracer|enabled, tracePattern+patterns|shouldTrace() via
TracingAdvice.before()|
|BaseProcessorSupport|disabled|CamelInternalProcessor.process() — every
exchange|
|DefaultBacklogDebugger|fallbackTimeout (long — non-atomic on 32-bit JVMs),
bodyMaxChars, bodyIncludeStreams, bodyIncludeFiles, includeExchangeProperties,
includeExchangeVariables,
singleStepIncludeStartEnd|NodeBreakpoint.beforeProcess()|
|TotalRequestsThrottler|timePeriodMillis (long)|process() — every exchange|
|AbstractThrottler|maxRequestsExpression|process() — every exchange|
|DefaultStreamCachingStrategy|spoolThreshold (long),
spoolUsedHeapMemoryThreshold, anySpoolRules, bufferSize|shouldSpoolCache()|
|ThrottlingInflightRoutePolicy|maxInflightExchanges+resumeInflightExchanges
(compound non-atomic), resumePercentOfMax, scope|throttle() via
onExchangeDone()|
|ThrottlingExceptionRoutePolicy|halfOpenAfter (long), failureWindow (long),
failureThreshold|calculateState() via onExchangeDone()|
|ManagedPerformanceCounter|statisticsEnabled|DefaultInstrumentationProcessor.before()
— every exchange|
|ScheduledPollConsumer|greedy, sendEmptyMessageWhenIdle, delay,
runLoggingLevel|doRun() on every poll cycle|
|Delayer|delay (Expression)|calculateDelay() — every exchange|
h3. Recommended fix approach
* For simple boolean/int fields: add volatile. Cost is negligible — on x86,
volatile reads compile to the same instruction as plain loads.
* For compound writes (tracePattern+patterns,
maxInflightExchanges+resumeInflightExchanges): volatile alone is insufficient.
These need either a lock or an immutable holder object swapped atomically via
volatile reference.
* Do NOT use synchronized on MBean getters/setters — it would add lock
contention on hot paths for no benefit.
h3. Practical impact
None of these are known to cause flaky tests today (only BacklogTracer's guard
conditions manifested, fixed in PR #24713). On x86 the bug is mostly
theoretical because volatile reads compile to plain loads. The fix is justified
as correctness-by-contract per the Java Memory Model, and would matter on
ARM/POWER architectures with weaker memory ordering.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
