allthingssecurity opened a new pull request, #26943: URL: https://github.com/apache/camel/pull/26943
# Description [CAMEL-25060](https://issues.apache.org/jira/browse/CAMEL-25060) A property placeholder that refers back to itself through an optional placeholder makes `DefaultPropertiesParser` loop for ever. The thread that resolves it, often the one starting the `CamelContext`, runs at 100 % CPU without any log: ``` self={{?self}} resolvePropertyPlaceholders("{{self}}") never returns a={{?b}}, b={{?a}} resolvePropertyPlaceholders("x{{a}}y") never returns timeout={{?timeout:5000}} from("timer:t?period={{timeout}}&repeatCount=1"), CamelContext.start() never returns loop1={{loop2}}, loop2={{loop1}} IllegalArgumentException: Circular reference detected with key [loop1] ... ``` `doParseNested` keeps the keys being resolved on the current branch in a set. When it meets one of them again it throws for a required key, but for an optional key it does `break` and returns the text with the placeholder still in it. The level above puts that text back into its `answer`, and its next `readProperty` finds the same placeholder first. The set of that level is one key smaller, so it resolves the placeholder again, gets the same text back, and so on. The `break` can never produce a result. The set only holds the keys above the current placeholder on the same branch (it is copied for each branch), so the check only fires for a real cycle. After a `break`, the level above either resolves the same key again into exactly the same state (when the key is not in its own set), or breaks as well (when it is), and the top level starts with an empty set. So, unless a properties function returns a different value on each call, no configuration gets a result through this path, and changing what it does cannot break a configuration that works today. This change removes the optional special case, so a circular reference through an optional placeholder fails with the same `IllegalArgumentException` ("Circular reference detected with key [?timeout:5000] from text: ...") as one through required placeholders. Why an error, and not "treat it like a missing optional key": - The docs ("Using optional property placeholders" in `using-propertyplaceholder.adoc`) describe what happens when an optional key is missing: it is dropped from the text, and an endpoint option that uses it is removed. They say nothing about cycles, and a key that points back to itself is not missing, it is a configuration error, as it is for a required key. - Treating it as missing would have to be done carefully to be useful: `{{?timeout:5000}}` would have to give the default `5000` (not an empty value), and in an endpoint URI the option would have to be removed (not left as `period=`). That is more code and more cases, for a configuration that is wrong, and the user would never learn about it. - The comment "Check for circular references (skip optional)" comes from CAMEL-16302 (3.9.0), which introduced optional placeholders. Skipping never worked, since the `break` hands the placeholder back to be read again; an error at startup is the closest terminating behaviour to what a required cycle does. To take a value from somewhere else and otherwise a default, another key works: `timeout={{?timeout.override:5000}}`. `PropertiesComponent.resolveProperty(key)` catches `IllegalArgumentException` and returns an empty `Optional`, so there such a cycle now gives "not found", as a required cycle already does, instead of a hang. Upgrade guide 4.23: a short section, since a configuration that used to hang now fails with an error. Tests, in a new `PropertiesComponentCircularReferenceTest`; each case runs in `assertTimeoutPreemptively(10 s)`: - `self={{?self}}` (through `{{self}}` and `{{?self}}`), `a={{?b}}`/`b={{?a}}` and `timeout={{?timeout:5000}}` each throw the circular reference error with the right key. - `from("timer:t?period={{timeout}}&repeatCount=1")` with `timeout={{?timeout:5000}}`: `CamelContext.start()` fails with the error in its causes. - `loop1={{loop2}}`/`loop2={{loop1}}` still throws (there was no test for the required cycle in camel-core). - Placeholders without a cycle are unchanged: `a={{b}}-{{?c}}` gives `B-`, `{{a}}{{a}}` gives `B-B-` (the same key twice is not a cycle), `x{{?nope}}y` gives `xy`, `{{?nope}}` gives `null`, `{{?nope:5000}}` gives `5000`, `timeout={{?override.timeout:5000}}` gives `5000`, and a nested missing optional key is dropped. - In an endpoint URI, `mock:result?retainFirst={{?maxKeep}}&resultWaitTime={{timeout}}` still drops the missing optional option and resolves the nested default (`mock://result?resultWaitTime=5000`). Without the change in `DefaultPropertiesParser`, the four cycle tests fail with `execution timed out after 10000 ms` (the three control tests pass). With it, the new test runs in under a second, and `*Properties*,*Placeholder*,*PropertyInject*` (188 tests) and `*RouteTemplate*,*Optional*,*Kamelet*,*PropertyBinding*,*Endpoint*` (580 tests) in camel-util, camel-base, camel-core and camel-console pass with 0 failures. Found with a Lean model of `readProperty` and `doParseNested`, which proves that resolving `{{self}}` with `self={{?self}}` runs out of fuel for every amount of fuel (one iteration of the loop at the second level returns to exactly the same state). I then reproduced the hang against the real classes, including a thread dump of `CamelContext.start()`. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested the affected modules, including the formatter and import-sort plugins. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
