[
https://issues.apache.org/jira/browse/CAMEL-25073?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123579#comment-18123579
]
shashank commented on CAMEL-25073:
----------------------------------
I went through the five items on current main (374c04877418), each with a JUnit
test against the real MBean server. All five reproduce. Two are clear bugs with
a small fix (PRs follow); three need a decision, options below.
*1. CamelContext name with {{, = : " * ?}}* - reproduced: {{start()}} fails
({{MalformedObjectNameException}} for {{, = : "}},
{{RuntimeOperationsException}} for {{*}} and {{?}}, as the unquoted value makes
the name a pattern); there is no fallback, the context does not start. A space
is fine.
Classification: needs a decision. Quoting the context key means
{{getContextId}} (all 20 names go through it) plus 13 hand-built
{{context=...}} queries in main code ({{ManagedCamelContext}} 7,
{{ManagedRoute}} 1, {{ProducerDevConsole}} 2, {{ReceiveDevConsole}} 1, the two
{{CamelRouteCoverageDumper}}), about 76 test references, and every external
query (hawtio, Jolokia, scripts) keeps breaking for such names.
Options:
* (a) quote only when needed, through one helper used by {{getContextId}} and
the 13 queries;
* (b) sanitize the management name when it is set in {{onContextStarting}}
(replace {{, = : " * ?}} and line breaks with {{_}}, WARN once) - about 5
lines, every existing query keeps working, only names that cannot start today
are affected; the context MBean still shows the real name in {{name=}} and
{{CamelId}};
* (c) fail fast with a clear message.
Recommendation: (b).
*2. Context-scoped onException* - reproduced: with routes {{a}} and {{b}} there
is one MBean for the onException's {{to}}, its {{RouteId}} is {{a}} and it
counts both routes (since CAMEL-25065 the managed object is cached per
definition, and the definition is the same instance in every route). Removing
route {{a}} unregisters it while {{b}} still counts into it.
Also found: the parent of that definition is the {{OnExceptionDefinition}},
whose parent is null, so {{removeWrappedProcessorsForRoutes}} never removes its
{{wrappedProcessors}} entries; with a context-scoped onException every removed
route leaves its onException processors in the map (a slow leak with routes
added/removed at runtime).
Classification: needs a decision.
Options:
* (a) one shared MBean, reference counted: record the route id per wrapped
processor (the intercept strategy is created per route), remove entries by that
id (fixes the leak), and only unregister when no other route uses the
definition - matches what CAMEL-25065 already does, no name change, but
{{RouteId}} shows the first route and the statistics are aggregated;
* (b) one MBean per route (route id in the name) - per-route statistics, but
MBean names change and the id lookups need work;
* (c) do not register these processors (like the {{OnExceptionDefinition}}
itself).
Recommendation: (a), the leak part could also go first on its own.
*3. Endpoints that only differ in a masked secret* - reproduced in two ways:
removing the endpoint of another route, and removing an endpoint with that name
created at runtime (never registered); both unregister the MBean of the first
endpoint.
Classification: clear bug. Fix: {{JmxManagementLifecycleStrategy}} keeps, per
endpoint MBean name it registered, the endpoint it registered it for, and does
not unregister the name when another endpoint is removed. The second endpoint
still has no MBean of its own (same name), as before. PR follows (branch
{{camel-management-masked-endpoint-unregister}}, new
{{ManagedRemoveEndpointMaskedSecretTest}}, fails without the fix).
*4. Thread pools of the same source* - reproduced: an aggregate with
{{completionTimeout}} and {{optimisticLocking}} creates two pools from the
{{AggregateProcessor}} (a recoverable repository adds a third), but only
{{name="AggregateProcessor(0x...)"}} is registered; the others are skipped as
"already managed" and are invisible in JMX. {{BaseExecutorServiceManager}}
builds the id from the source only, not from the pool name.
Classification: needs a decision, as MBean names change.
Options:
* (a) for a source that is neither a {{NamedNode}} nor a {{String}}, use the
pool name as the sourceId:
{{AggregateProcessor(0x...)(AggregateTimeoutChecker)}} (the existing
{{id(sourceId)}} format); the names that change contain an identity hash today;
* (b) put the pool name into the id itself;
* (c) disambiguate on a clash in the lifecycle strategy (order dependent);
* (d) document.
Recommendation: (a), with an upgrade note for the {{SourceId}} attribute.
*5. Components with {{mbeansLevel=ContextOnly}}* - reproduced: {{direct}},
{{mock}}, {{seda}} (and a component added after start) are registered.
{{onComponentAdd}} does not use {{shouldRegister}}, where CAMEL-18085 added the
level check.
Classification: clear bug. Fix: skip the registration when the level does not
include routes; {{RoutesOnly}} and {{Default}} unchanged; upgrade-guide note.
The health check and route controller MBeans that are registered next to the
CamelContext MBean are left as they are. PR follows (branch
{{camel-management-contextonly-components}}).
The two PRs reference this ticket and say which item they address. I'll send
PRs for 1, 2 and 4 once you pick an option.
_Claude Code on behalf of allthingssecurity_
> camel-management - MBean registration: follow-ups from the deep review
> ----------------------------------------------------------------------
>
> Key: CAMEL-25073
> URL: https://issues.apache.org/jira/browse/CAMEL-25073
> Project: Camel
> Issue Type: Bug
> Components: camel-management
> Reporter: Claus Ibsen
> Priority: Minor
>
> Follow-ups found in the review of CAMEL-25065
> (https://github.com/apache/camel/pull/26959), not changed there:
> # A CamelContext name (or management name pattern) with , = : " * ? fails to
> start, as the context key of the object names is not quoted (every query that
> builds the context key would have to change too).
> # A context-scoped onException is one definition in every route, so its
> processors share one MBean, which is unregistered when one of the routes is
> removed.
> # Two endpoints that only differ in a masked secret get the same object name;
> removing one unregisters the other's MBean.
> # Thread pools created by the same source (such as the aggregate's recover
> and timeout checkers) get the same object name.
> # Components are registered with mbeansLevel=ContextOnly.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
