jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3851027127
##########
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:
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).
##########
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:
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.
--
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]
