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