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.
##########
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.
--
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]
