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