[jira] [Resolved] (CAMEL-24227) Add volatile to JMX-writable fields read on routing threads

["Guillaume Nodet (Jira)" <ji...@apache.org>]

     [ 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)