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