jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3860502088
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -57,6 +61,19 @@ public class OsgiTypeConverter extends ServiceSupport
implements TypeConverter,
private CamelContext camelContext;
private final Injector injector;
private final ServiceTracker<TypeConverterLoader, Object> tracker;
+ /**
+ * The loaders the tracker has handed us, kept here rather than read back
from the tracker: resolving them
+ * through the tracker inside {@link #createRegistry()} would mean calling
into the ServiceTracker and the
+ * framework while holding this instance's monitor.
+ */
+ private final Map<ServiceReference<TypeConverterLoader>,
TypeConverterLoader> trackedLoaders
+ = new ConcurrentHashMap<>();
+ /**
+ * Registrations made through this facade rather than by a {@link
TypeConverterLoader}, in the order they were
+ * made, so a rebuilt registry can be brought back to the same state.
Discarding the delegate would otherwise
+ * drop them with no way to get them back.
+ */
+ private final List<Consumer<TypeConverterRegistry>>
programmaticRegistrations = new CopyOnWriteArrayList<>();
Review Comment:
**Blocking.** This list is unbounded and never pruned, which in OSGi means
classloader retention.
Every registration ever made through the facade is retained for the life of
the `CamelContext`, and each retained lambda strongly references the
`TypeConverter` instance it captured — and therefore the contributing bundle's
classloader. After that bundle is uninstalled, nothing here lets go. That is
the leak shape this repo cares most about, and it is new in this PR: before,
discarding the delegate did at least drop the references.
Reproduced: 500 `addTypeConverter`/`removeTypeConverter` pairs leave the
list at **1000** entries, with the converter from pair 1 still strongly
reachable.
Two knock-ons worth naming:
- **Rebuild cost grows monotonically.** Every rebuild replays the whole
list, and `addTypeConverters(Object)` re-runs reflective `@Converter` scanning
each time. Since #739 also holds the instance monitor across the rebuild, the
container's conversion stall grows with the number of registrations the context
has ever seen.
- **The list has no relationship to bundle lifecycle.** A Blueprint
container refresh re-registers, appending duplicates rather than replacing.
A keyed collection — `Map<TypeConvertible<?,?>,
Consumer<TypeConverterRegistry>>`, insertion-ordered — would fix growth,
retention, and the `removeTypeConverter` ordering question in one move: a
removal deletes the entry instead of appending an inverse. It does not solve
`addTypeConverters(Object)`, which has no natural key, but that is a much
smaller surface to think about.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -112,6 +150,16 @@ public void
removedService(ServiceReference<TypeConverterLoader> serviceReferenc
this.delegate = null;
Review Comment:
**Blocking**, though I would rather see it split out than folded in here —
see the bottom.
This write is not under the monitor that `getDelegate()` now takes, so
making `getDelegate()` `synchronized` buys mutual exclusion between *readers*
only, never against the writer. The invalidation can be swallowed entirely:
```
A: getDelegate() takes `this`, delegate == null, enters createRegistry()
(loader L already loaded into the new registry)
B: removedService(L) -> stopService(this.delegate) // still null, no-op
-> this.delegate = null // no monitor held
A: delegate = <registry built with L in it>
```
I reproduced this by stalling at the end of `createRegistry()`: after
`removedService` returns, `getDelegate()` hands back *the same instance* that
was built with the removed loader in it. Assertion `assertSame(built,
afterRemoval)` passes. So converters belonging to an unregistered bundle keep
serving indefinitely, and no subsequent event will invalidate them.
This predates the PR — the old unsynchronized `if (delegate == null)` had
the same lost write. But it sits squarely inside #734's point 1, and
`synchronized getDelegate()` makes the window *wider*, not narrower: the
monitor is now held across the full rebuild (core converter scan plus every
`loader.load()`), so there is much more time for a `removedService` to land
inside it.
An epoch counter would close it — read a generation before
`createRegistry()`, re-check it before the assign, discard and retry if
`removedService` bumped it meanwhile.
That said: this is the same underlying problem as the eager-stop race you
raised on the `getDelegate()` thread — invalidation and stop both happening
outside the monitor, to an instance callers already hold. I would rather have
one follow-up issue covering both than see either bolted on here. Your call,
but if you open that issue, put this in it and I will not hold the PR on it.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +335,59 @@ public Set<ClassLoader> getClassLoaders() {
throw new RuntimeCamelException("Error loading CoreTypeConverter
due: " + e.getMessage(), e);
}
- // Load the type converters the tracker has been tracking
- // Here we need to use the ServiceReference to check the ranking
- ServiceReference<TypeConverterLoader>[] serviceReferences =
this.tracker.getServiceReferences();
- if (serviceReferences != null) {
- ArrayList<ServiceReference<TypeConverterLoader>> servicesList =
- new ArrayList<>(Arrays.asList(serviceReferences));
- // Just make sure we install the high ranking fallback converter
at last
- Collections.sort(servicesList);
- for (ServiceReference<TypeConverterLoader> sr : servicesList) {
- try {
- LOG.debug("loading type converter from bundle: {}",
sr.getBundle().getSymbolicName());
-
((TypeConverterLoader)this.tracker.getService(sr)).load(answer);
- } catch (Throwable t) {
- throw new RuntimeCamelException("Error loading type
converters from service: " + sr + " due: " + t.getMessage(), t);
- }
+ // Load the type converters the tracker has been tracking. These come
from our own map rather than from
+ // tracker.getServiceReferences()/getService(): this runs while
holding this instance's monitor, and
+ // calling back into the tracker from here is what would establish a
lock ordering against the framework.
+ List<ServiceReference<TypeConverterLoader>> servicesList = new
ArrayList<>(trackedLoaders.keySet());
+ // Just make sure we install the high ranking fallback converter at
last
+ Collections.sort(servicesList);
+ for (ServiceReference<TypeConverterLoader> sr : servicesList) {
+ TypeConverterLoader loader = trackedLoaders.get(sr);
+ if (loader == null) {
+ // unregistered between the snapshot and here
+ continue;
+ }
+ try {
+ LOG.debug("loading type converter from bundle: {}",
sr.getBundle().getSymbolicName());
+ loader.load(answer);
+ } catch (Throwable t) {
+ throw new RuntimeCamelException("Error loading type converters
from service: " + sr + " due: " + t.getMessage(), t);
}
}
+ replayProgrammaticRegistrations(answer);
+
LOG.trace("Created TypeConverter: {}", answer);
return answer;
}
+ /**
+ * Re-applies everything that was registered through this facade rather
than by a
+ * {@link TypeConverterLoader}, in the order it was originally applied.
+ */
+ private void replayProgrammaticRegistrations(DefaultTypeConverter
registry) {
+ if (programmaticRegistrations.isEmpty()) {
+ return;
+ }
+ LOG.debug("Replaying {} programmatic registration(s) onto the rebuilt
type converter registry",
+ programmaticRegistrations.size());
+ for (Consumer<TypeConverterRegistry> registration :
programmaticRegistrations) {
+ registration.accept(registry);
+ }
+ }
+
+ /**
+ * Applies a registration to the current delegate and remembers it, so
that discarding the delegate does not
+ * discard the registration with it.
+ */
+ private void register(Consumer<TypeConverterRegistry> registration) {
+ // apply first: a registration the delegate rejects is not one worth
replaying. Note getDelegate() may
+ // build the registry here, which replays the list as it stands - this
registration is added after, so
+ // it cannot be applied twice
+ registration.accept(getDelegate());
+ programmaticRegistrations.add(registration);
+ }
Review Comment:
**Blocking.** This reintroduces the bug the PR fixes, one level down.
`register()` is apply-then-record, and no monitor spans both halves. A
rebuild landing between them loses the registration from the live registry even
though it is sitting in the replay list:
```
A: addTypeConverter -> getDelegate() returns D1 -> stalls inside the apply
B: removedService -> delegate = null
C: getDelegate() -> builds D2, replays the list (r is not in it yet)
A: programmaticRegistrations.add(r)
=> r is in the replay list, but was only ever applied to the discarded D1
```
I reproduced this against this head with a registry whose `addTypeConverter`
I could stall inside: after the sequence above, `d2.lookup(String.class,
Marker.class)` returns `null`, and no further rebuild happens to correct it. So
the converter is silently absent from the live registry — exactly the symptom
`programmaticConverterSurvivesARegistryRebuild` is meant to rule out.
The comment here is right that the registration cannot be applied *twice*;
the gap is that it can be applied *zero* times.
The window is narrow for `addTypeConverter`, but `addTypeConverters(Object)`
does reflective `@Converter` scanning, and Blueprint bean registration runs
concurrently with bundle lifecycle events, so it is not theoretical.
Fix is cheap, since `getDelegate()` is reentrant on the same monitor:
```java
private synchronized void register(Consumer<TypeConverterRegistry>
registration) {
```
`removeTypeConverter` has the same split and needs the same treatment.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -121,6 +169,8 @@ protected void doStart() throws Exception {
protected void doStop() throws Exception {
this.tracker.close();
this.trackerOpened = false;
+ // close() calls removedService for everything still tracked, this
only makes the end state explicit
+ this.trackedLoaders.clear();
ServiceHelper.stopService(this.delegate);
this.delegate = null;
}
Review Comment:
**Blocking**, and a one-liner.
`trackedLoaders` is cleared here but `programmaticRegistrations` is not, so
a stop/start cycle replays registrations from the previous lifecycle —
including converters owned by bundles that are gone by the time the context
comes back up.
Reproduced against this head: `start()` -> `addTypeConverter(String, Marker,
tc)` -> `stop()` -> `start()` -> `getDelegate().lookup(String.class,
Marker.class)` returns the converter from the previous lifecycle.
`ServiceSupport` permits stop/start, and `OsgiTypeConverter` is a
context-scoped service, so this is reachable on a context restart. Clearing the
list alongside `trackedLoaders` is enough, and it would be worth a test next to
`removedServiceReleasesTheService`.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -238,10 +291,13 @@ public TypeConverterExists getTypeConverterExists() {
@Override
public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
- getDelegate().setTypeConverterExists(typeConverterExists);
+ register(registry ->
registry.setTypeConverterExists(typeConverterExists));
}
- public DefaultTypeConverter getDelegate() {
+ // fully synchronized rather than double checked: the delegate is not
immutable after publication -
+ // removedService stops and replaces it - so a lock free read of the field
buys a race for no real gain,
+ // conversion work dwarfing an uncontended monitor either way
+ public synchronized DefaultTypeConverter getDelegate() {
Review Comment:
Not blocking, but the invariant the safety argument rests on does not hold,
so I would like the comment and the PR description to stop asserting it.
The PR body says there is "no call into the tracker or the framework left
underneath this instance's monitor at all". There are several. `getDelegate()`
is now `synchronized` and calls `ensureTrackerOpen()`, and `tracker.open()`
does `bundleContext.addServiceListener(...)` and
`bundleContext.getServiceReferences(...)`, then synchronously runs
`trackInitial()` -> your own `addingService` -> `bundleContext.getService(...)`
— all under `this`. Separately, `createRegistry()` runs foreign `loader.load()`
under `this`.
No ABBA results, because the callbacks no longer take this monitor, and (per
your disassembly, which I verified) `Tracked` is not held across the customizer
either. So the code is *not* deadlock-prone. But "we removed the `tracker.*`
calls from `createRegistry()`" and "nothing framework-facing runs under our
monitor" are different claims, and only the first one is true. Holding `this`
across arbitrary bundle `load()` is the same shape I objected to originally; it
is now reached by a different route.
Second, on this comment specifically: "conversion work dwarfing an
uncontended monitor" is true in steady state and misleading during a rebuild,
which is the case that matters. While `createRegistry()` runs — core converter
package scan plus every tracked loader's `load()` — every conversion in the
container is blocked on this monitor, and per the `programmaticRegistrations`
thread that duration grows over the life of the context.
I am fine with the tradeoff. Please just say what it is: all conversions
serialize behind a rebuild, and the monitor is held across foreign bundle code.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -173,27 +223,30 @@ public <T> T tryConvertTo(Class<T> type, Object value) {
@Override
public void addTypeConverter(Class<?> toType, Class<?> fromType,
TypeConverter typeConverter) {
- getDelegate().addTypeConverter(toType, fromType, typeConverter);
+ register(registry -> registry.addTypeConverter(toType, fromType,
typeConverter));
}
@Override
public void addTypeConverters(Object typeConverters) {
- getDelegate().addTypeConverters(typeConverters);
+ register(registry -> registry.addTypeConverters(typeConverters));
}
@Override
public void addBulkTypeConverters(BulkTypeConverters bulkTypeConverters) {
- getDelegate().addBulkTypeConverters(bulkTypeConverters);
+ register(registry ->
registry.addBulkTypeConverters(bulkTypeConverters));
}
@Override
public boolean removeTypeConverter(Class<?> toType, Class<?> fromType) {
- return getDelegate().removeTypeConverter(toType, fromType);
+ boolean removed = getDelegate().removeTypeConverter(toType, fromType);
+ // replayed as well, so a rebuild reproduces the sequence rather than
resurrecting the converter
+ programmaticRegistrations.add(registry ->
registry.removeTypeConverter(toType, fromType));
+ return removed;
}
Review Comment:
Two things here, both feeding the retention problem in the
`programmaticRegistrations` thread.
**Appends an inverse instead of pruning.** Recording a *removal* rather than
deleting the matching *add* means the removed converter stays strongly
reachable forever — the add lambda still holds it. Replay reproduces the right
end state, so the tests pass, but the object is never released. Deleting the
entry would give the same end state and actually let go.
**Appends even when nothing was removed.** `removed` is computed and
returned but not consulted. A `removeTypeConverter` for a pair that was never
registered still grows the list and still costs a no-op call on every future
rebuild.
Also, as on the `register()` thread: the `getDelegate()` call and the `add`
are not atomic with respect to each other, so this has the same
lost-registration window.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/test/java/org/apache/camel/karaf/core/OsgiTypeConverterTest.java:
##########
@@ -17,7 +17,19 @@
package org.apache.camel.karaf.core;
import org.apache.camel.CamelContext;
+import org.apache.camel.RuntimeCamelException;
+import org.apache.camel.TypeConverter;
import org.apache.camel.spi.Injector;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
Review Comment:
Style: the `java.util.*` imports landed after the `org.apache.camel.*`
block, splitting it in two. Everywhere else in this file and in
`OsgiTypeConverter.java` the order is `java.*`, blank line, `org.*`. Please
move these above `org.apache.camel.CamelContext`.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -203,7 +256,7 @@ public TypeConverter lookup(Class<?> toType, Class<?>
fromType) {
@Override
public void setInjector(Injector injector) {
- getDelegate().setInjector(injector);
+ register(registry -> registry.setInjector(injector));
}
Review Comment:
Minor: `setInjector` / `setTypeConverterExists` /
`setTypeConverterExistsLoggingLevel` are *state*, not events, and replaying
them as ordered lambdas gets the ordering subtly wrong.
`createRegistry()` constructs `OsgiDefaultTypeConverter` with the
constructor-time `injector` field, runs `init()` and
`loadCoreAndFastTypeConverters()`, and only then replays. So a rebuilt registry
loads its core converters under the *old* injector and swaps afterwards — not
equivalent to a registry built fresh with the current one. `this.injector` is
never updated by this setter either.
Still an improvement on losing them entirely, so not blocking. But holding
them as plain fields and applying them at construction would be both simpler
and correct, and would keep three entries out of the replay list.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +84,36 @@ public OsgiTypeConverter(BundleContext bundleContext,
CamelContext camelContext,
this.tracker = new ServiceTracker<>(bundleContext,
TypeConverterLoader.class.getName(), this);
}
- private void ensureTrackerOpen() {
+ private synchronized void ensureTrackerOpen() {
if (!trackerOpened) {
tracker.open();
trackerOpened = true;
}
}
Review Comment:
Agreed on keeping this, and your narrower justification is the right one.
One nit while you are in here, pre-existing but now more reachable: if
`tracker.open()` throws, `trackerOpened` stays `false`, but
`ServiceTracker.open()` assigns `tracked` *before* calling `trackInitial()`, so
the retry returns early and never re-runs `trackInitial()`. The initial loaders
that had not been processed yet are then never tracked at all.
`addingService` can throw `RuntimeCamelException` out of `trackInitial()`
when a loader's `load()` fails, so one bad loader at startup can silently
strand the others. Worth a separate issue rather than growing this PR —
flagging it because the method is in the diff.
_Claude Code on behalf of JB Onofré_
--
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]
