jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3851027125
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,15 +67,15 @@ 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;
}
}
@Override
- public Object addingService(ServiceReference<TypeConverterLoader>
serviceReference) {
+ public synchronized Object
addingService(ServiceReference<TypeConverterLoader> serviceReference) {
LOG.trace("AddingService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
TypeConverterLoader loader =
bundleContext.getService(serviceReference);
if (loader != null) {
Review Comment:
**Finding 5 — OSGi use-count leaked when `loader.load()` throws.**
`bundleContext.getService()` increments the framework use-count for this
service reference. If `loader.load(delegate)` subsequently throws,
`addingService()` propagates `RuntimeCamelException`. The `ServiceTracker`
treats a throwing `addingService()` as "not tracked", so `removedService()` is
never called for this reference and `bundleContext.ungetService()` is never
invoked.
The use-count is permanently stuck above zero; the originating bundle cannot
be cleanly uninstalled for the lifetime of the OSGi framework. Fix: wrap
`loader.load()` in a try-catch and call
`bundleContext.ungetService(serviceReference)` in the catch before re-throwing.
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -242,15 +251,26 @@ public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
}
public DefaultTypeConverter getDelegate() {
- if (delegate == null) {
- // ensure the tracker is open so we can discover
TypeConverterLoader services
Review Comment:
**Finding 2 — DCL fast path can return a stopped delegate.**
The fast path reads `answer = delegate` (volatile) and, if non-null, skips
the `synchronized` block and returns `answer` directly to the caller. However,
a concurrent `removedService()` (holding `synchronized(this)`) can call
`ServiceHelper.stopService(this.delegate)` and set `this.delegate = null`
*after* Thread A's null-check but *before* Thread A uses the returned value.
Thread A then calls conversion methods on a stopped converter, resulting in
`IllegalStateException` or silent data loss.
DCL is only safe when the published object is truly immutable after
publication. A delegate that can be stopped and replaced does not meet that
bar. Consider always going through the lock in `getDelegate()` — type
conversion is not so hot that the lock overhead is measurable compared to
actual conversion work.
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -242,15 +251,26 @@ public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
}
public DefaultTypeConverter getDelegate() {
- if (delegate == null) {
- // ensure the tracker is open so we can discover
TypeConverterLoader services
- // before creating the registry - this is important because
getDelegate() may be
- // called during doInit() (e.g. when to() eagerly creates
endpoints) which happens
- // before doStart() where the tracker is normally opened
- ensureTrackerOpen();
- delegate = createRegistry();
+ DefaultTypeConverter answer = delegate;
+ if (answer == null) {
+ // double checked locking against the volatile field: getDelegate
is on the conversion hot path,
+ // so the common case must stay lock free, but the check and the
assignment together are not
+ // atomic - without the lock two threads racing on first access
each build a registry, and
+ // whatever was registered on the one that loses is silently
dropped
+ synchronized (this) {
+ answer = delegate;
+ if (answer == null) {
+ // ensure the tracker is open so we can discover
TypeConverterLoader services
+ // before creating the registry - this is important
because getDelegate() may be
+ // called during doInit() (e.g. when to() eagerly creates
endpoints) which happens
+ // before doStart() where the tracker is normally opened
+ ensureTrackerOpen();
+ answer = createRegistry();
Review Comment:
**Finding 1 — ABBA lock-order inversion; deadlock under concurrent service
events.**
`createRegistry()` is called here while holding `synchronized(this)`. Inside
`createRegistry()`, it calls `tracker.getServiceReferences()` and
`tracker.getService()`, which the Felix/Equinox `ServiceTracker` guards with
its own internal `Tracked` monitor.
Meanwhile, the OSGi framework holds that same `Tracked` monitor when
dispatching service events — it calls `addingService()` / `removedService()`,
which this PR has made `synchronized(this)`.
Thread A: holds `synchronized(this)` → calls `tracker.getService()` → waits
for `Tracked` monitor
Thread B: holds `Tracked` monitor → calls `addingService()` → waits for
`synchronized(this)`
Classic ABBA deadlock. This lock order did not exist before this PR. Fix:
snapshot the tracked services *outside* any lock (e.g. call
`tracker.getTracked()` which returns a defensive copy, or move the `tracker.*`
calls before entering `synchronized(this)`), then use the snapshot inside the
lock.
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -100,8 +100,17 @@ public void
modifiedService(ServiceReference<TypeConverterLoader> serviceReferen
}
@Override
- public void removedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
+ public synchronized void
removedService(ServiceReference<TypeConverterLoader> serviceReference, Object
o) {
LOG.trace("RemovedService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
+ if (this.delegate != null) {
+ // the rebuild in createRegistry replays the core converters and
the loaders the tracker still
Review Comment:
**Finding 6 — Warning acknowledges the loss of programmatic converters but
does not fix it.**
When `removedService()` stops the entire delegate and `createRegistry()`
rebuilds it, only converters discovered via the `ServiceTracker` are replayed.
Converters registered programmatically via `addTypeConverter()`,
`addBulkTypeConverters()`, or `addFallbackTypeConverter()` — including
Blueprint bean-registered converters — are silently dropped with the old
delegate.
Routes relying on those converters will fail with
`NoTypeConversionAvailableException` after any bundle uninstalls a
`TypeConverterLoader`, with no diagnostic pointing to the root cause. The log
warning is insufficient. Either:
- Maintain a separate replay list of programmatically registered converters
and re-apply them in `createRegistry()`.
- Abandon full-delegate teardown in favour of partial removal (unloading
only the converters contributed by the departing loader).
--
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]
