allthingssecurity opened a new pull request, #27464: URL: https://github.com/apache/camel/pull/27464
# Description [CAMEL-25073](https://issues.apache.org/jira/browse/CAMEL-25073), item 2 @davsclaus listed this after his deep review of #26959 (CAMEL-25065). The outputs of a context scoped `onException` are the same definition instance in every route (`RouteDefinitionHelper.initOnExceptions`). Since CAMEL-25065 caches the managed object per definition, all routes share one processor MBean. Removing one route unregistered that MBean, although the other routes still counted into it: their statistics went to an object that was no longer registered. The ticket has three options. This PR implements the recommended one, (a), with the re-attach that the TLA+ model showed it needs (see the follow-up comment on the ticket). The alternatives are listed below; if you prefer one of them, I'll change the PR. The same happens for the outputs of a context scoped `onCompletion` and of the `onException` of a route configuration: they are also one definition shared by the routes that use them. The change covers them too. Also fixed: the parent of such a definition is the `OnExceptionDefinition` (or `OnCompletionDefinition`, `RouteConfigurationDefinition`), not a route, so `removeWrappedProcessorsForRoutes` never matched it. Every removed route left these processors (with their `InstrumentationProcessor`) in the `wrappedProcessors` map. That is a slow leak when routes are added and removed at runtime (route templates, Kamelets, reloads). Change, all in `camel-management`: - `InstrumentationInterceptStrategy` is created per route in `onRouteContextCreate`. It now records the route of each wrapped processor: a small `WrappedProcessor` record replaces the `KeyValueHolder`, and the constructor takes the route. The class is only created by `JmxManagementLifecycleStrategy`; there is no other use in the Camel repositories. - `removeWrappedProcessorsForRoutes` removes the entries of the removed route by that route. It keeps the cached managed object of a shared definition while another route still has a processor for it. - `onServiceRemove` does not unregister the MBean of a processor while a processor of another route was created from the same definition. The kept MBean is attached to that processor. If it showed the removed route, it is registered again with the remaining route. So `RouteId`, `State`, start/stop and the agent's per-route processor index (used by the route dumps) follow the route that still uses it. Definitions that belong to their route (the normal case) take the same path as before, without scanning the map. The check runs only when a route is removed, never per message. `wrappedProcessors` is read there while the model and context locks are held, so it does not race with a route being added or removed. Options on the ticket: - (a) One shared MBean, reference counted (this PR). No MBean name changes. As before, the statistics are aggregated over the routes, and `RouteId` shows one of the routes that use it. - (b) One MBean per route (route id in the name). Gives per-route statistics, but MBean names change, and `processorsById`, `dumpRouteStatsAsXml` and the TUI look processors up by id. Needs an upgrade note. - (c) Do not register these processors, like the `OnExceptionDefinition` itself. Their statistics are lost. Found and checked with a TLA+ model of the lifecycle of that MBean (`SharedProcessorMBean.tla`, kept outside the repo). Two threads add routes (model lock, processors wrapped without the context lock), start and stop them (context lock only, as the supervising route controller or JMX do) and remove them (model lock, then context lock), and a route can be added again. TLC runs with deadlock checking. On main it violates "every started route counts into the registered MBean" (add a, add b, remove a) and "no wrapped processors left after all routes are removed" (already with one route). Option (a) as first described (reference count only) passes those but violates "a kept MBean shows a live route and processor": after removing the route the MBean was created for, `RouteId` stays the removed route and `State` shows `Stopped`. With the re-attach it passes all properties for 2 and 3 routes and with a route scoped onException (one definition per route, the negative control), with no doub le registration in any configuration. I ran it again on the rebased branch; nothing that the model covers changed on main. No upgrade guide entry: nothing that worked before behaves differently. Tests: - New `ManagedRouteRemoveContextScopedOnExceptionTest` (remove route a, remove route b, remove both). - New `ManagedRouteRemoveSharedProcessorTest` (a context scoped onCompletion and a route configuration's onException; remove route a). All four fail on main in two runs (`The onException processor mbean should still be registered for route b ==> expected: <true> but was: <false>`, `No wrapped processors should be left after removing all routes ==> expected: <0> but was: <2>`). Without the re-attach part, two of the onException tests still fail (`expected: <b> but was: <a>`, `expected: <Started> but was: <Stopped>`). Also checked by hand, not in a test, as they behave the same on main: a context stop and start, and a route stop and start before the removal. The whole `camel-management` module passes (535 tests, 0 failures, 1 skipped). Not changed (noted on the ticket): `wrappedProcessors` is a plain `HashMap`. Route creation writes it under the model lock while a route start on another thread can read it under the context lock only. A `ConcurrentHashMap` would avoid that; I have no test for it, so it is left for a separate change. Related CAMEL-25073 PRs, each for one item and based on `main`: item 1 (`camel-management-context-name-object-name`), item 3 (`camel-management-masked-endpoint-unregister`), item 4 (`camel-management-thread-pool-source-id`) and item 5 (`camel-management-contextonly-components`). All five merge cleanly with `main` and with each other in every pair (`git merge-tree`). Items 1, 2, 3 and 5 change different methods of `JmxManagementLifecycleStrategy`; items 1, 4 and 5 add separate paragraphs under `=== camel-management` in the upgrade guide. With all five merged together, the `camel-management` module passes (553 tests, 0 failures, 1 skipped). None depends on another. # 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 `core/camel-management`, including the formatter and import-sort plugins. No generated files change. 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]
