jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3860502075
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +335,59 @@ 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());
+ loader.load(answer);
+ } catch (Throwable t) {
+ throw new RuntimeCamelException("Error loading type converters
from service: " + sr + " due: " + t.getMessage(), t);
}
}
+ replayProgrammaticRegistrations(answer);
+
LOG.trace("Created TypeConverter: {}", answer);
return answer;
}
+ /**
+ * Re-applies everything that was registered through this facade rather
than by a
+ * {@link TypeConverterLoader}, in the order it was originally applied.
+ */
+ private void replayProgrammaticRegistrations(DefaultTypeConverter
registry) {
+ if (programmaticRegistrations.isEmpty()) {
+ return;
+ }
+ LOG.debug("Replaying {} programmatic registration(s) onto the rebuilt
type converter registry",
+ programmaticRegistrations.size());
+ for (Consumer<TypeConverterRegistry> registration :
programmaticRegistrations) {
+ registration.accept(registry);
+ }
+ }
+
+ /**
+ * Applies a registration to the current delegate and remembers it, so
that discarding the delegate does not
+ * discard the registration with it.
+ */
+ private void register(Consumer<TypeConverterRegistry> registration) {
+ // apply first: a registration the delegate rejects is not one worth
replaying. Note getDelegate() may
+ // build the registry here, which replays the list as it stands - this
registration is added after, so
+ // it cannot be applied twice
+ registration.accept(getDelegate());
+ programmaticRegistrations.add(registration);
+ }
Review Comment:
**Blocking.** This reintroduces the bug the PR fixes, one level down.
`register()` is apply-then-record, and no monitor spans both halves. A
rebuild landing between them loses the registration from the live registry even
though it is sitting in the replay list:
```
A: addTypeConverter -> getDelegate() returns D1 -> stalls inside the apply
B: removedService -> delegate = null
C: getDelegate() -> builds D2, replays the list (r is not in it yet)
A: programmaticRegistrations.add(r)
=> r is in the replay list, but was only ever applied to the discarded D1
```
I reproduced this against this head with a registry whose `addTypeConverter`
I could stall inside: after the sequence above, `d2.lookup(String.class,
Marker.class)` returns `null`, and no further rebuild happens to correct it. So
the converter is silently absent from the live registry — exactly the symptom
`programmaticConverterSurvivesARegistryRebuild` is meant to rule out.
The comment here is right that the registration cannot be applied *twice*;
the gap is that it can be applied *zero* times.
The window is narrow for `addTypeConverter`, but `addTypeConverters(Object)`
does reflective `@Converter` scanning, and Blueprint bean registration runs
concurrently with bundle lifecycle events, so it is not theoretical.
Fix is cheap, since `getDelegate()` is reentrant on the same monitor:
```java
private synchronized void register(Consumer<TypeConverterRegistry>
registration) {
```
`removeTypeConverter` has the same split and needs the same treatment.
--
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]
