[
https://issues.apache.org/jira/browse/CAMEL-24227?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Guillaume Nodet resolved CAMEL-24227.
-------------------------------------
Resolution: Fixed
> 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
> Assignee: Guillaume Nodet
> Priority: Minor
> Fix For: 4.22.0
>
>
> 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)
