CAMEL-24267: Synchronize writer-side setters in ThrottlingInflightRoutePolicy - #25161
Merged
gnodet merged 2 commits intoJul 29, 2026
Merged
Conversation
…tePolicy setMaxInflightExchanges and setResumePercentOfMax each read the other's field to recompute the ThrottlingLimits holder. Without synchronization, concurrent JMX callers can each read a stale value from the other setter, producing a holder built from a mixed snapshot. Synchronize both methods to ensure writer-side atomicity (JMX-invoked, so contention is negligible). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
davsclaus
approved these changes
Jul 27, 2026
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 553 tested, 29 compile-only — current: 553 all testedMaveniverse Scalpel detected 582 affected modules (current approach: 553).
|
oscerd
approved these changes
Jul 28, 2026
gnodet
marked this pull request as ready for review
July 28, 2026 16:44
gnodet
commented
Jul 28, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
Author
There was a problem hiding this comment.
Review: Looks good ✅
Clean, well-justified follow-up to PR #24985 (CAMEL-24227). Adding synchronized to both JMX-writable setters correctly addresses the writer-side race condition where concurrent callers could each read a stale snapshot of the other's field, producing a ThrottlingLimits holder with inconsistent values. The test is thorough and CI is green.
Observations:
-
The production code change is minimal, correct, and well-scoped. The
synchronizedkeyword on both setters provides mutual exclusion so that each setter reads a consistent value of the other's field when recomputing theThrottlingLimitsholder. Combined with the existingvolatileon the holder reference (from CAMEL-24227), this gives a clean asymmetric synchronization pattern: writers exclude each other viasynchronized, while readers (throttle()) get visibility via the singlevolatileread of the immutable holder. -
The concurrency test using
CyclicBarrieris a well-known technique for verifying concurrent correctness. The 5,000-iteration loop with varyingmaxValandpctValprovides good coverage of different interleavings. The single-threaded and edge-case (clamping) tests are good supplementary coverage.
Minor nit: java.lang.reflect.Method is used as a fully qualified class name in the test body (lines 59, 113, 148) while java.lang.reflect.Field is properly imported. The project convention says to always add an import statement and use the simple class name. Consider adding import java.lang.reflect.Method;.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| */ | ||
| package org.apache.camel.throttling; | ||
|
|
||
| import java.lang.reflect.Field; |
Contributor
Author
There was a problem hiding this comment.
Minor style nit: java.lang.reflect.Method is used as a FQCN in the test body (lines 59, 113, 148) while Field is properly imported here. Per project convention ("Do NOT use fully qualified class names"), consider adding:
Suggested change
| import java.lang.reflect.Field; | |
| import java.lang.reflect.Field; | |
| import java.lang.reflect.Method; |
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in 4770d50. Added import java.lang.reflect.Method and replaced all inline FQCNs with the simple name.
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
commented
Jul 28, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
Author
There was a problem hiding this comment.
Review: Looks good ✅
Re-review after the FQCN fix commit — the import java.lang.reflect.Method was correctly added and all five FQCN usages replaced with the simple name across the three test methods. Clean fix, no other issues.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
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 gnodet
setMaxInflightExchangesandsetResumePercentOfMaxto prevent concurrent JMX callers from producing aThrottlingLimitsholder with mixed stale values. Both setters read the other's field to recompute the resume limit — without synchronization, two concurrent callers can each read a stale snapshot, resulting in an inconsistent holder.ThrottlingInflightRoutePolicySetterTest) that hammers both setters from separate threads with aCyclicBarrierand verifies the invariant: the publishedThrottlingLimitsalways reflects a consistent(max, resume)pair.synchronizedis the simplest correct solution.JIRA: CAMEL-24267
Follow-up to: PR #24985 (CAMEL-24227) which fixed reader-side tearing but left writer-side atomicity unaddressed.
What changed
ThrottlingInflightRoutePolicy.javasynchronizedto both setter methodsThrottlingInflightRoutePolicySetterTest.javaTest plan
ThrottlingInflightRoutePolicySetterTestpasses (3 tests)ThrottlingInflightRoutePolicyTestpasses🤖 Generated with Claude Code