[
https://issues.apache.org/jira/browse/CAMEL-25073?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123591#comment-18123591
]
shashank commented on CAMEL-25073:
----------------------------------
Before sending PRs I checked my recommendations for items 1 and 2 with small
formal models (Lean for item 1, TLA+ for item 2). Both change details of what I
wrote above, so here is the correction before you pick an option.
h3. Item 1 (CamelContext name with {{, = : " * ?}})
*What the model found*
* Option (b) as I described it is not safe on its own. {{a,b}} and {{a_b}}
sanitize to the same management name, and {{a_b}} starts today. I wrote that
the existing clash handling covers this, but it does not: the clash check
compares the whole context object name, which includes the quoted real name
({{context=a_b,type=context,name="a,b"}} vs {{...name="a_b"}}). So both
contexts start with the context key {{a_b}}, in either order. The route,
processor and component object names of the second context are then already
taken, and {{registerMBeanWithServer}} skips them silently. The second context
gets no MBeans, its {{ManagedCamelContext}} queries see the first context's
MBeans, and stopping it can unregister them.
* No sanitizing can avoid this, whatever the replacement: a function that keeps
every name that works today and maps the other names to valid names can never
be injective (if {{x}} is invalid, {{g(x)}} is valid, so {{g(g(x)) = g(x)}}).
Collisions have to be handled when the name is registered.
* The same gap exists today without special characters. Two contexts with
different names and the same fixed {{managementNamePattern}} both start with
the same context key (no veto, checked with a test), and so do {{foo}}, a
second {{foo}} (which gets {{foo-1}}) and then a context named {{foo-1}}.
* Option (a) has no such problem. The {{ObjectName.quote}}/{{unquote}} round
trip holds for every string, quote-only-when-needed keeps every name that works
today, and it is injective (a quoted value starts with {{"}}, which a working
name never contains). Its cost is the same as before: {{getContextId}} plus the
13 queries built by hand, plus external tooling.
* Only the line feed has to be replaced, not every line break: {{\r}} works
unquoted.
*Updated recommendation*
(b), but only together with a clash check on the context key: a CamelContext
MBean with the same {{context=}} key and another name counts as a clash. Then
the next free management name is used, or the start is vetoed with a fixed
pattern, as already happens for two contexts with the same name. In the model
this keeps the context keys unique for every start sequence, and it gives the
same result as main wherever main was right (a veto, or keys that are already
unique). It is about 20 more lines in {{JmxManagementLifecycleStrategy}} and
one {{context=<key>,type=context,*}} query on the MBean server. If you prefer
the real name in the {{context=}} key, (a) is the option to take.
I have this ready locally, pending your decision: sanitizing plus the
context-key check, a test for the 7 characters (the route MBeans are found
through {{ManagedCamelContext}}, {{CamelId}} keeps the real name),
{{a,b}}/{{a_b}} in both orders, and a fixed pattern used by two names (now
vetoed). All of these fail on main. The camel-management suite passes, and
there is an upgrade guide paragraph. I'll open the PR once you choose between
(a) and (b).
h3. Item 2 (context-scoped onException)
I modelled two threads adding, starting, stopping, removing and re-adding
routes under the real model and context locks (TLA+, TLC with deadlock checking
on).
* On main the model shows both problems from my comment: the shared MBean is
unregistered while another route still counts into it (add a, add b, remove a),
and the {{wrappedProcessors}} entries leak even with a single route.
* Option (a) *as I described it* (reference count plus route id per entry)
fixes both, but is not enough: when the first route is removed, the kept MBean
stays attached to that removed route, so {{RouteId}} shows the removed route,
{{State}} shows {{Stopped}}, and start/stop act on the dead processor.
* Option (a) *plus re-attaching the MBean to a remaining route* when its route
is removed passes every property (2 and 3 routes, re-adds, the route-scoped
control), with no double registration. The reference check runs with both locks
held, so it does not race with route adds/removes.
* In Java: three tests reproduce it on main; without the re-attach two of them
still fail.
Updated recommendation: (a) with the re-attach. I have it ready locally,
pending your decision (camel-management suite passes, no upgrade note needed).
Model-only side note, not changed: {{wrappedProcessors}} is a plain {{HashMap}}
that route creation writes while a route start on another thread can read it;
low risk, I can look at it separately.
_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)
