jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608021
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +358,73 @@ 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);
Review Comment:
**The replay is outside any try/catch, so one throwing registration
permanently bricks the registry — and with `TypeConverterExists.Fail` it is
guaranteed to throw.**
I disassembled `CoreTypeConverterRegistry` from `camel-base-4.18.1.jar`:
- `addConverter(TypeConvertible, TypeConverter)` is a bare
`converters.put(...)` — no duplicate check.
- `addTypeConverter(Class, Class, TypeConverter)` routes through
`addOrReplaceTypeConverter()` -> `onTypeConverterExists()`, which returns
`true` for `Override`, `false` for `Ignore`, and otherwise **throws
`TypeConverterExistsException`**.
So:
1. Context is configured `typeConverterExists=Fail`.
2. A Blueprint bean calls `addTypeConverter(Foo, Bar, tc)` while only core
converters are loaded — no conflict, applied and recorded.
3. Bundle X's `TypeConverterLoader` later registers `Bar -> Foo` via
`addConverter()` (plain `put`, no check).
4. Any loader unregisters; the delegate is discarded.
5. Next `getDelegate()`: loaders load **first**, then this replay calls
`addTypeConverter(Foo, Bar, tc)` -> existing converter found ->
`TypeConverterExistsException` escapes `createRegistry()` and `getDelegate()`.
`delegate` stays `null`, so every `convertTo` / `tryConvertTo` / `lookup` in
the container throws forever, and each retry re-throws because the offending
entry is still in `programmaticRegistrations`. The loader loop just above is
wrapped in `try/catch (Throwable)`; the replay is not.
Even on the default path this is lossy rather than fatal: the constructor
sets `typeConverterExists = Ignore` and `typeConverterExistsLoggingLevel =
LoggingLevel.DEBUG`, so the same collision silently drops the programmatic
converter and logs only at DEBUG.
`programmaticConverterSurvivesARegistryRebuild` does not catch it because the
`Marker` interface is chosen specifically so the pair cannot collide with a
core converter.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +358,73 @@ 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());
Review Comment:
**`sr.getBundle().getSymbolicName()` is dereferenced eagerly inside the
`try`, so a concurrently-stopping bundle turns a debug log into an aborted
registry rebuild.**
`ServiceReference.getBundle()` returns `null` once the service is
unregistered. This loop already anticipates that window — the `loader == null`
guard four lines up is commented "unregistered between the snapshot and here" —
and then dereferences `getBundle()` unconditionally for the same window. The
argument is evaluated regardless of log level, so DEBUG being off does not help.
Because the call sits inside the `try { ... } catch (Throwable t)` that
rethrows as `RuntimeCamelException`, the NPE does not just skip one loader: it
escapes `createRegistry()` and `getDelegate()`, so a routine bundle stop makes
the whole type converter registry unbuildable.
Worth noting the inconsistency: `removedService` guards this exact call
(`serviceReference.getBundle() != null ? ... : serviceReference`), but this
line and the one in `addingService` do not. The tests mask it because `setUp()`
stubs `serviceReference.getBundle()` to a non-null mock.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -99,9 +138,21 @@ public Object
addingService(ServiceReference<TypeConverterLoader> serviceReferen
public void modifiedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
}
+ // not synchronized, for the same reason as addingService
@Override
public void removedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
LOG.trace("RemovedService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
+ trackedLoaders.remove(serviceReference);
+ // we took the service in addingService, so releasing it is ours to do
+ ungetQuietly(serviceReference);
+ if (this.delegate != null && !isStopping() && !isStopped()) {
Review Comment:
**`!isStopped()` also suppresses this warning in every pre-`STARTED` state,
so the diagnostic #734 asked for is missing exactly in the lazy-init window
this class documents.**
From `BaseService` (camel-api 4.18):
```java
public boolean isStopped() {
return status < STARTING || status >= STOPPED;
}
```
with `NEW = 0, BUILT = 1, INITIALIZING = 2, INITIALIZED = 3, STARTING = 4
... STOPPED = 9 ... FAILED = 12`.
So `isStopped()` is `true` for `NEW`, `BUILT`, `INITIALIZING`, `INITIALIZED`
and `FAILED` — not just after a stop. `getDelegate()`'s own comment below says
it "may be called during `doInit()` ... which happens before `doStart()`". An
`OsgiTypeConverter` that is already serving conversions but has not reached
`STARTED` will therefore discard its registry on a loader unregistration with
**no log line at all**, which is the invisibility this PR exists to remove.
`isStopping()` alone already achieves the stated goal (no line-per-loader on
`tracker.close()`, since `stop()` sets `status = STOPPING` before calling
`doStop()`). The `&& !isStopped()` half is what breaks it; if a post-stop guard
is also wanted, `status != STOPPED` is the narrow form.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -238,10 +314,13 @@ public TypeConverterExists getTypeConverterExists() {
@Override
public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
- getDelegate().setTypeConverterExists(typeConverterExists);
+ register(TYPE_CONVERTER_EXISTS_KEY, 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:
**Making `getDelegate()` fully synchronized turns the conversion hot path
into a global serialization point, and blocks every conversion in the container
for the full duration of a rebuild.**
`convertTo`, `tryConvertTo`, `mandatoryConvertTo`, `lookup`, `allowNull`,
`size`, `getStatistics` and `getInjector` all funnel through here. Previously
`delegate` was `volatile` and read without a lock, so the hot path was
contention-free; now every conversion on every route thread contends one
monitor (and biased locking is gone as of JDK 15, so it inflates).
The rebuild case is the sharper one: when a single `TypeConverterLoader`
unregisters, the next `getDelegate()` holds this monitor across `init()` +
`loadCoreAndFastTypeConverters()` (bundle resource scanning) + `loader.load()`
for every tracked loader + the replay. Every conversion in the container blocks
for that whole span. `registrationCannotInterleaveWithARebuild` demonstrates
precisely this with its 2 s stall.
The comment's "conversion work dwarfing an uncontended monitor" only holds
while uncontended. Keeping a volatile fast-path read and synchronizing only the
build gives the same build-once guarantee at no hot-path cost:
```java
DefaultTypeConverter d = delegate;
if (d != null) {
return d;
}
synchronized (this) { ... }
```
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -173,27 +242,33 @@ 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(new TypeConvertible<>(fromType, toType),
+ registry -> registry.addTypeConverter(toType, fromType,
typeConverter));
}
@Override
public void addTypeConverters(Object typeConverters) {
- getDelegate().addTypeConverters(typeConverters);
+ register(typeConverters, registry ->
registry.addTypeConverters(typeConverters));
Review Comment:
**The keyed map only prunes the `addTypeConverter` / `addConverter` pair.
`addTypeConverters`, `addBulkTypeConverters` and `addFallbackTypeConverter` are
keyed by contributed-instance identity and have no removal API, so the map
still grows without bound and still pins bundle classloaders.**
`removeTypeConverter` is the only pruning path and it only deletes a
`TypeConvertible` key. `TypeConverterRegistry` exposes no
`removeTypeConverters` / `removeFallbackTypeConverter` /
`removeBulkTypeConverters`, so entries recorded here (and at the
`addBulkTypeConverters` / `addFallbackTypeConverter` calls below) live until
`doStop()`.
That breaks the stated rationale for keying on the affected path.
"Re-registering replaces rather than appends, which is what a Blueprint
container refresh does" is true for `addTypeConverter`, but a container refresh
instantiates a **new bean**, so `addTypeConverters(newBeanInstance)` hashes to
a fresh identity key and appends. 50 refreshes of a bundle contributing a
`TypeConverters` bean leave 50 entries, each strongly holding the bean and
through it the classloader of a bundle that may already be uninstalled — the
leak shape the review flagged, on the path the PR description names as its
motivating case. `addRemovePairsDoNotAccumulate` only exercises
`addTypeConverter` / `removeTypeConverter`.
Secondary point: all three share one untyped key space, and
`BulkTypeConverters extends Ordered, TypeConverter`, so the same instance
passed to both `addBulkTypeConverters` and `addFallbackTypeConverter` collides
and one registration is dropped from the replay.
(`CoreTypeConverterRegistry.addBulkTypeConverters` is also a bare `return` in
4.18, so recording that one is dead weight either way.)
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -99,9 +138,21 @@ public Object
addingService(ServiceReference<TypeConverterLoader> serviceReferen
public void modifiedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
}
+ // not synchronized, for the same reason as addingService
Review Comment:
**This comment is anchored here, but the defect is `this.delegate = null` at
line 163 just below (and the `ServiceHelper.stopService(this.delegate)` above
it) — an unsynchronized write to a field `getDelegate()` now guards with a
monitor, so an invalidation can be swallowed by an in-flight rebuild.**
Interleaving:
1. T1 in synchronized `getDelegate()`: `delegate == null`, enters
`createRegistry()`, snapshots `trackedLoaders` (contains `sr_A`), begins
loading.
2. T2 (framework dispatch, unsynchronized `removedService` for `sr_A`):
removes `sr_A` from `trackedLoaders`, calls
`ServiceHelper.stopService(this.delegate)` on the still-null field, sets
`delegate = null`.
3. T1 finishes and assigns `delegate` = the registry it built **with**
`sr_A`'s converters.
The invalidation is lost and nothing will trigger another rebuild, so the
context permanently serves converters from a bundle that is gone. The
mirror-image interleaving hands a stopped delegate to a live caller.
I see this is split out to #743, which is reasonable as a scoping call — but
widening `getDelegate()` to a full monitor in *this* PR makes the window
larger, not smaller, so it is worth confirming the deferral still holds rather
than inheriting it from the pre-monitor shape.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -173,27 +242,33 @@ 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(new TypeConvertible<>(fromType, toType),
+ registry -> registry.addTypeConverter(toType, fromType,
typeConverter));
}
@Override
public void addTypeConverters(Object typeConverters) {
- getDelegate().addTypeConverters(typeConverters);
+ register(typeConverters, registry ->
registry.addTypeConverters(typeConverters));
}
@Override
public void addBulkTypeConverters(BulkTypeConverters bulkTypeConverters) {
- getDelegate().addBulkTypeConverters(bulkTypeConverters);
+ register(bulkTypeConverters, registry ->
registry.addBulkTypeConverters(bulkTypeConverters));
}
@Override
- public boolean removeTypeConverter(Class<?> toType, Class<?> fromType) {
- return getDelegate().removeTypeConverter(toType, fromType);
+ public synchronized boolean removeTypeConverter(Class<?> toType, Class<?>
fromType) {
+ boolean removed = getDelegate().removeTypeConverter(toType, fromType);
+ // delete the matching registration rather than recording an inverse.
An inverse would reproduce the same
+ // end state on replay, but it would also keep the removed converter -
and its bundle's classloader -
+ // strongly reachable for the life of the context, and cost a no-op
call on every future rebuild
+ programmaticRegistrations.remove(new TypeConvertible<>(fromType,
toType));
+ return removed;
}
@Override
public void addFallbackTypeConverter(TypeConverter typeConverter, boolean
canPromote) {
- getDelegate().addFallbackTypeConverter(typeConverter, canPromote);
+ register(typeConverter, registry ->
registry.addFallbackTypeConverter(typeConverter, canPromote));
Review Comment:
**Replaying after the loader loop inverts fallback converter precedence,
because `addFallbackTypeConverter` prepends.**
`CoreTypeConverterRegistry.addFallbackTypeConverter` (verified in bytecode)
is:
```java
fallbackConverters.add(0, new FallbackTypeConverter(typeConverter,
canPromote));
```
i.e. **last added == index 0 == tried first**. That is what
`createRegistry()`'s "Just make sure we install the high ranking fallback
converter at last" comment relies on: the highest-ranking `ServiceReference`
sorts last, so its fallback lands at index 0.
`replayProgrammaticRegistrations(answer)` then runs *after* the entire loop,
so any programmatic fallback is prepended in front of the high-ranking loader
fallback that the sort deliberately put there.
In the live registry the opposite ordering held: a fallback registered at
time T sat behind every loader that arrived after T (`addingService` ->
`loader.load(delegate)` -> prepend). So a single loader unregistration silently
flips which fallback wins for every unmatched conversion in the context. The
field Javadoc's claim that the rebuilt registry is brought "back to the same
state" does not hold for fallbacks.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +97,36 @@ public OsgiTypeConverter(BundleContext bundleContext,
CamelContext camelContext,
this.tracker = new ServiceTracker<>(bundleContext,
TypeConverterLoader.class.getName(), this);
}
- private void ensureTrackerOpen() {
+ private synchronized void ensureTrackerOpen() {
Review Comment:
**`ensureTrackerOpen()` is now synchronized on `this`, but `doStop()` closes
the tracker and clears `trackerOpened` without that monitor — and
`getDelegate()` has no lifecycle guard, so the tracker can be left open after
stop, permanently.**
`BaseService.stop()` runs `doStop()` under its own `ReentrantLock`, not
under `this`, so the two are not mutually excluded. Interleaving: T1 in
`doStop()` executes `tracker.close()`; T2 inside synchronized `getDelegate()`
-> `ensureTrackerOpen()` calls `tracker.open()`; T1 then sets `trackerOpened =
false`. Result: a live, listening `ServiceTracker` on a `STOPPED` service,
holding a `getService()` use count for every `TypeConverterLoader`.
Nothing can clean that up — `BaseService.stop()` returns early on `if
(status == STOPPED || status == SHUTTING_DOWN || status == SHUTDOWN)`, so a
second `stop()` never reaches `doStop()`.
The same hole exists with no race at all: `getDelegate()` performs no
`isStopped()` / `isStopping()` check, so one stray conversion after stop calls
`ensureTrackerOpen()` -> `tracker.open()` and resurrects the tracker for the
life of the framework, pinning every contributing bundle's classloader.
Synchronizing one half of a two-field invariant is worse than neither, because
it reads as protected. `restartDoesNotReplayThePreviousLifecycle` shows the
lifecycle was considered, but only the explicit stop/start path.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +97,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;
}
}
+ // deliberately not synchronized: the tracker calls this from the
framework's service event dispatch, and
+ // taking this instance's monitor here would put our lock on the far side
of the framework's, which is the
+ // ordering that makes a lock inversion possible
@Override
public Object addingService(ServiceReference<TypeConverterLoader>
serviceReference) {
LOG.trace("AddingService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
TypeConverterLoader loader =
bundleContext.getService(serviceReference);
if (loader != null) {
+ trackedLoaders.put(serviceReference, loader);
try {
LOG.debug("loading type converter from bundle: {}",
serviceReference.getBundle().getSymbolicName());
- if (delegate != null) {
+ DefaultTypeConverter current = delegate;
Review Comment:
**`delegate` is read here without the monitor, so a loader arriving during a
rebuild can end up recorded in `trackedLoaders` but present in no registry,
with nothing left to trigger another rebuild.**
1. T1 in synchronized `getDelegate()`: `delegate == null`, enters
`createRegistry()`, executes `new ArrayList<>(trackedLoaders.keySet())` — empty.
2. T2 (framework dispatch, `addingService` for `sr_A`):
`trackedLoaders.put(sr_A, loader)` succeeds, then this line reads `delegate` as
`null` because T1 has not assigned yet, so the `if (current != null)` branch is
skipped and `load()` is never called.
3. T1 completes and publishes a registry that does not contain `sr_A`'s
converters.
`sr_A` is in `trackedLoaders`, so no further service event fires and the
next rebuild only happens on an unrelated unregistration. Until then every
conversion `sr_A` provided fails with `NoTypeConversionAvailableException`.
Recording the loader and deciding whether to load it are two steps raced
against a build that is atomic with neither. Note the pre-PR code had the same
hole via `tracker.getServiceReferences()` (the tracker's `tracked` map is only
populated after `addingService` returns), so this is not a regression — but it
is in the method being rewritten, and the `trackedLoaders` map is now the place
where it could actually be closed.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +358,73 @@ 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.
Review Comment:
**This invariant does not hold. Anchoring here because the comment states
it; the calls that break it are `ensureTrackerOpen()` inside synchronized
`getDelegate()` (line 329) and `loader.load(answer)` / `answer.init()` in this
method.**
The comment here, plus the ones on `addingService` and `removedService`,
justify unsynchronizing the callbacks and abandoning
`tracker.getServiceReferences()` on the grounds that no framework call may
happen under this monitor. But inside synchronized `getDelegate()`:
- `ensureTrackerOpen()` -> `ServiceTracker.open()` acquires framework
service-registry locks **and synchronously dispatches `addingService` for every
initial service**;
- `answer.init()` / `answer.loadCoreAndFastTypeConverters()` do
bundle-wiring resource scanning;
- `loader.load(answer)` twelve lines below runs arbitrary third-party bundle
code that may call back into the framework or into this same
`OsgiTypeConverter`.
So the `this` -> framework-lock edge the redesign was meant to remove is
still present, just relocated. A concrete shape: the framework dispatch thread
runs `addingService` -> `loader.load(current)` where a `CamelContextAware`
loader performs a conversion -> `getDelegate()` -> blocks on `this`, while
another thread holds `this` inside `tracker.open()`.
The residual risk may well be acceptable, but the comments assert the
mitigation is complete when it is partial, which is the part that will mislead
the next reader.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +358,73 @@ 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.values()) {
+ registration.accept(registry);
+ }
+ }
+
+ /**
Review Comment:
**Two consecutive Javadoc blocks: this one is orphaned by the `/** */` for
`programmaticRegistrationCount()` that follows, so `register()` ends up with no
Javadoc and the surviving comment documents the wrong method.**
Java attaches only the last block before a declaration. This block — the
load-bearing explanation of why apply-and-record has to be atomic, which both
the commit message and the PR description lean on — becomes a dangling comment
attached to nothing, and `register()` at line 421 is undocumented. The `{@link
#getDelegate()}` reference in it is invisible to doclint too.
Looks like a rebase/squash artifact; moving this block down to immediately
precede `register()` fixes it.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/test/java/org/apache/camel/karaf/core/OsgiTypeConverterTest.java:
##########
@@ -17,7 +17,20 @@
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;
Review Comment:
**The new `java.util` imports were inserted into the middle of the existing
`org.apache.camel` group, splitting it in two.**
Lines 19-22 are `org.apache.camel.*`, lines 23-31 are `java.util.*`, then
line 33 returns to `org.apache.camel.impl.converter.DefaultTypeConverter` and
`org.apache.camel.spi.*`.
`OsgiTypeConverter.java` itself (lines 19-28) and the rest of the module put
`java.*` first, then a blank line, then the third-party groups. There is no
enforced formatter plugin here so nothing fails the build, but this guarantees
a churn diff the next time anyone runs an IDE import organiser over the file.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -203,7 +278,7 @@ public TypeConverter lookup(Class<?> toType, Class<?>
fromType) {
@Override
public void setInjector(Injector injector) {
Review Comment:
**`setInjector` only records a replay lambda and never updates the
`injector` field, so every rebuilt registry is constructed with the
constructor-time injector and loads its core converters before the replay
corrects it.**
`createRegistry()` passes the `final` field `this.injector` to `new
OsgiDefaultTypeConverter(...)`, then calls `answer.init()` and
`answer.loadCoreAndFastTypeConverters()`, and only afterwards calls
`replayProgrammaticRegistrations(answer)`. So after a rebuild, converters
instantiated during core loading used the **old** injector; the configured one
is swapped in only after the fact, and anything already created keeps the stale
reference.
`TYPE_CONVERTER_EXISTS_KEY` and `TYPE_CONVERTER_EXISTS_LOGGING_LEVEL_KEY`
have the same ordering problem, and it compounds the replay issue on
`replayProgrammaticRegistrations`: a context configured `Override` or `Fail`
has all its core and loader converters registered under the constructor
defaults (`Ignore` / `DEBUG`), and the real policy only takes effect partway
through the replay, in insertion order.
This is strictly better than pre-PR behaviour (where the setting was lost
entirely), so not a blocker — but "brought back to the same state" is not what
happens.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/test/java/org/apache/camel/karaf/core/OsgiTypeConverterTest.java:
##########
@@ -110,4 +125,243 @@ void removedServiceShouldInvalidateDelegate() throws
Exception {
var delegateAfter = osgiTypeConverter.getDelegate();
assertNotNull(delegateAfter);
}
+
+ @Test
+ void concurrentFirstAccessShouldBuildTheRegistryOnce() throws Exception {
+ int threads = 16;
+ AtomicInteger created = new AtomicInteger();
+ CountDownLatch startLine = new CountDownLatch(1);
+
+ OsgiTypeConverter counting = new OsgiTypeConverter(bundleContext,
camelContext, injector) {
+ @Override
+ protected DefaultTypeConverter createRegistry() {
+ created.incrementAndGet();
+ return super.createRegistry();
+ }
+ };
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+ try {
+ List<Future<DefaultTypeConverter>> futures = new ArrayList<>();
+ for (int i = 0; i < threads; i++) {
+ futures.add(pool.submit(() -> {
+ startLine.await();
+ return counting.getDelegate();
+ }));
+ }
+ // release them all at once so they race on the null check
+ startLine.countDown();
+
+ DefaultTypeConverter first = futures.get(0).get(30,
TimeUnit.SECONDS);
+ assertNotNull(first);
+ for (Future<DefaultTypeConverter> f : futures) {
+ assertSame(first, f.get(30, TimeUnit.SECONDS),
+ "every caller must see the same registry instance");
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+
+ assertEquals(1, created.get(),
+ "the registry must be built exactly once, otherwise converters
registered on a discarded"
+ + " instance are silently lost");
+ }
+
+ /** Marker source type, so the registered converter cannot collide with a
core one. */
+ interface Marker {
+ }
+
+ @Test
+ void rebuiltRegistryReloadsTheTrackedLoadersWithoutAskingTheTracker()
throws Exception {
+ // arrives before the registry exists, so addingService only records it
+ osgiTypeConverter.addingService(serviceReference);
+ verify(loader, never()).load(any());
+
+ DefaultTypeConverter first = osgiTypeConverter.getDelegate();
+
+ // createRegistry replayed it from the recorded loaders; it never
called back into the ServiceTracker,
+ // which is what would put a framework call underneath this instance's
monitor
+ verify(loader).load(first);
+ }
+
+ @Test
+ void programmaticConverterSurvivesARegistryRebuild() throws Exception {
+ DefaultTypeConverter before = osgiTypeConverter.getDelegate();
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ assertNotNull(before.lookup(String.class, Marker.class),
"precondition: the converter is registered");
+
+ // one loader going away discards the whole registry
+ osgiTypeConverter.removedService(serviceReference, loader);
+ DefaultTypeConverter after = osgiTypeConverter.getDelegate();
+
+ assertNotSame(before, after, "the registry should have been rebuilt");
+ assertNotNull(after.lookup(String.class, Marker.class),
+ "a converter registered programmatically must be replayed onto
the rebuilt registry, otherwise it"
+ + " disappears from a running context when any bundle
unregisters a loader");
+ }
+
+ @Test
+ void removedTypeConverterIsNotResurrectedByARebuild() throws Exception {
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+
+ osgiTypeConverter.removedService(serviceReference, loader);
+
+ assertNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class),
+ "the replay must reproduce the sequence, not just the
additions");
+ }
+
+ @Test
+ void addingServiceReleasesTheServiceWhenLoadingFails() throws Exception {
+ osgiTypeConverter.getDelegate();
+ doThrow(new RuntimeException("boom")).when(loader).load(any());
+
+ assertThrows(RuntimeCamelException.class, () ->
osgiTypeConverter.addingService(serviceReference));
+
+ // a customizer that throws is treated as never tracked, so
removedService will not run for this
+ // reference and nothing else would release the use count taken by
addingService
+ verify(bundleContext).ungetService(serviceReference);
+ }
+
+ @Test
+ void removedServiceReleasesTheService() {
+ osgiTypeConverter.addingService(serviceReference);
+
+ osgiTypeConverter.removedService(serviceReference, loader);
+
+ verify(bundleContext).ungetService(serviceReference);
+ }
+
+ @Test
+ void addingServiceMustNotHoldTheInstanceMonitor() throws Exception {
+ // build first, so addingService takes the branch that calls into the
loader
+ osgiTypeConverter.getDelegate();
+
+ CountDownLatch insideLoad = new CountDownLatch(1);
+ CountDownLatch releaseLoad = new CountDownLatch(1);
+ doAnswer(invocation -> {
+ insideLoad.countDown();
+ releaseLoad.await(30, TimeUnit.SECONDS);
+ return null;
+ }).when(loader).load(any());
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ Future<?> adding = pool.submit(() ->
osgiTypeConverter.addingService(serviceReference));
+ assertTrue(insideLoad.await(30, TimeUnit.SECONDS), "addingService
should have reached loader.load");
+
+ // the framework calls addingService while it is dispatching a
service event; if it took this
+ // instance's monitor, every conversion in the container would
block behind an arbitrary bundle's
+ // loader for as long as that loader takes
+ Future<DefaultTypeConverter> reader =
pool.submit(osgiTypeConverter::getDelegate);
+ assertNotNull(reader.get(10, TimeUnit.SECONDS),
+ "getDelegate must not be blocked by an in-flight
addingService");
+
+ releaseLoad.countDown();
+ adding.get(30, TimeUnit.SECONDS);
+ } finally {
+ releaseLoad.countDown();
+ pool.shutdownNow();
+ }
+ }
+
+ @Test
+ void addRemovePairsDoNotAccumulate() {
+ osgiTypeConverter.getDelegate();
+
+ for (int i = 0; i < 500; i++) {
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+ }
+
+ // an inverse-appending list would sit at 1000 here, and every
converter it captured - and the classloader
+ // of the bundle that contributed it - would stay strongly reachable
for the life of the context
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount(),
+ "add/remove pairs must prune, not accumulate");
+ }
+
+ @Test
+ void reRegisteringTheSameConversionReplaces() {
+ osgiTypeConverter.getDelegate();
+
+ for (int i = 0; i < 10; i++) {
+ // what a Blueprint container refresh looks like: the same
conversion registered again
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ }
+
+ assertEquals(1, osgiTypeConverter.programmaticRegistrationCount(),
+ "re-registering the same conversion must replace rather than
append");
+ }
+
+ @Test
+ void removingAConversionThatWasNeverRegisteredDoesNotAccumulate() {
+ osgiTypeConverter.getDelegate();
+
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount(),
+ "a removal for a pair that was never registered must not be
retained");
+ }
+
+ @Test
+ void restartDoesNotReplayThePreviousLifecycle() throws Exception {
+ osgiTypeConverter.start();
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ assertNotNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class));
+
+ osgiTypeConverter.stop();
+ osgiTypeConverter.start();
+
+ // the converters captured before the stop belong to bundles that may
be gone by now
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount());
+ assertNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class),
+ "a stop/start cycle must not resurrect the previous
lifecycle's registrations");
+ }
+
+ @Test
+ void registrationCannotInterleaveWithARebuild() throws Exception {
+ CountDownLatch insideBuild = new CountDownLatch(1);
+ CountDownLatch releaseBuild = new CountDownLatch(1);
+
+ OsgiTypeConverter stalling = new OsgiTypeConverter(bundleContext,
camelContext, injector) {
+ @Override
+ protected DefaultTypeConverter createRegistry() {
+ DefaultTypeConverter built = super.createRegistry();
+ insideBuild.countDown();
+ try {
+ releaseBuild.await(30, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ return built;
+ }
+ };
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ Future<DefaultTypeConverter> builder =
pool.submit(stalling::getDelegate);
+ assertTrue(insideBuild.await(30, TimeUnit.SECONDS), "the rebuild
should have started");
+
+ Future<?> registrar = pool.submit(() -> {
+ stalling.addTypeConverter(String.class, Marker.class,
typeConverter);
+ return null;
+ });
+
+ // apply-and-record has to be one step against the rebuild. If it
were not, this registration would be
+ // applied to the registry being discarded and only recorded
afterwards, so the rebuilt one would
+ // neither have it applied nor replay it
+ assertThrows(TimeoutException.class, () -> registrar.get(2,
TimeUnit.SECONDS),
Review Comment:
**Proving this by waiting 2 s for a `TimeoutException` is a `Thread.sleep`
in disguise, and it passes vacuously if the second pool thread has not yet
entered `addTypeConverter`.**
Two problems:
1. It adds a hard 2 s to every build and asserts a negative by elapsed time
— the pattern the project's Awaitility guidance exists to eliminate ("flaky,
slow, and non-deterministic").
2. It is not actually a guard. On a loaded CI box or with a cold thread
pool, the `registrar` thread may not have reached the synchronized `register()`
at all; the `get()` times out anyway and the test reports success even against
an implementation where `register()` is **not** synchronized. The PR
description already notes this test passes against its predecessor.
A deterministic positive assertion is available and the test almost makes it
already: release the build, join both futures, then assert the registration is
present in the published registry — which lines 358-359 do. Dropping the timing
assertion loses nothing and removes the 2 s.
_AI-generated review 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]
