oscerd commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3870770117
##########
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:
Fixed exactly as you suggested — `register()` is `private synchronized`, and
`removeTypeConverter()` has the same treatment.
You are right that the comment defended the wrong half. It argued the
registration cannot be applied *twice*; the hole was that it can be applied
*zero* times. I have rewritten it to say what the monitor is actually for.
On testing it: `registrationCannotInterleaveWithARebuild` stalls inside an
overridden `createRegistry()` and asserts a concurrent `addTypeConverter`
cannot proceed, then that it lands on the live registry. Being straight about
its limits — it passes against the previous head too, because the old
`register()` still went through a `synchronized getDelegate()` and blocked
there anyway. So it is a forward guard, not a regression test. Reproducing the
real interleave needs a registry that can be stalled inside its own
`addTypeConverter`, the way your throwaway did; I did not build one, and said
so in the commit message rather than let the test imply more coverage than it
has.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -112,6 +150,16 @@ public void
removedService(ServiceReference<TypeConverterLoader> serviceReferenc
this.delegate = null;
Review Comment:
Split out as you asked: **#743**, which covers both this lost invalidation
and the eager-stop race from the `getDelegate()` thread, since they are the
same shape — mutation of an instance callers already hold, performed outside
the monitor that publishes it — and a fix for one constrains the fix for the
other.
Your epoch-counter sketch is in there, along with your reproduction
(stalling at the end of `createRegistry()`, `assertSame(built, afterRemoval)`
passing) and the point that #739 widens the window rather than narrowing it,
since the monitor is now held across the whole rebuild.
Nothing about it is fixed in this PR.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -57,6 +61,19 @@ public class OsgiTypeConverter extends ServiceSupport
implements TypeConverter,
private CamelContext camelContext;
private final Injector injector;
private final ServiceTracker<TypeConverterLoader, Object> tracker;
+ /**
+ * The loaders the tracker has handed us, kept here rather than read back
from the tracker: resolving them
+ * through the tracker inside {@link #createRegistry()} would mean calling
into the ServiceTracker and the
+ * framework while holding this instance's monitor.
+ */
+ private final Map<ServiceReference<TypeConverterLoader>,
TypeConverterLoader> trackedLoaders
+ = new ConcurrentHashMap<>();
+ /**
+ * Registrations made through this facade rather than by a {@link
TypeConverterLoader}, in the order they were
+ * made, so a rebuilt registry can be brought back to the same state.
Discarding the delegate would otherwise
+ * drop them with no way to get them back.
+ */
+ private final List<Consumer<TypeConverterRegistry>>
programmaticRegistrations = new CopyOnWriteArrayList<>();
Review Comment:
Agreed, and it is my regression — the replay list introduced it. Taken your
suggestion.
`programmaticRegistrations` is now a keyed, insertion-ordered
`LinkedHashMap<Object, Consumer<TypeConverterRegistry>>`. The key is what the
registration is *about*:
- `TypeConvertible` for `addTypeConverter` and `addConverter`
- the contributed instance for `addBulkTypeConverters`,
`addFallbackTypeConverter` and `addTypeConverters(Object)`
- a sentinel per setter, so repeated `setTypeConverterExists` calls replace
rather than stack
That fixes all three things you named in one move: growth, retention, and
the `removeTypeConverter` ordering question. It also answers the Blueprint
refresh point — re-registering the same conversion now replaces the entry
instead of appending a duplicate (`reRegisteringTheSameConversionReplaces`).
You are right that `addTypeConverters(Object)` has no natural key. Keying on
the contributed object itself is the best available: a refresh that hands over
an equal instance replaces, and one that hands over a genuinely different
object is a genuinely different registration. It does not bound the reflective
`@Converter` re-scan on rebuild, which stays proportional to the number of
distinct contributed objects.
Tests: `addRemovePairsDoNotAccumulate` reproduces your figure — 1000 entries
against the previous head, 0 now.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -173,27 +223,30 @@ public <T> T tryConvertTo(Class<T> type, Object value) {
@Override
public void addTypeConverter(Class<?> toType, Class<?> fromType,
TypeConverter typeConverter) {
- getDelegate().addTypeConverter(toType, fromType, typeConverter);
+ register(registry -> registry.addTypeConverter(toType, fromType,
typeConverter));
}
@Override
public void addTypeConverters(Object typeConverters) {
- getDelegate().addTypeConverters(typeConverters);
+ register(registry -> registry.addTypeConverters(typeConverters));
}
@Override
public void addBulkTypeConverters(BulkTypeConverters bulkTypeConverters) {
- getDelegate().addBulkTypeConverters(bulkTypeConverters);
+ register(registry ->
registry.addBulkTypeConverters(bulkTypeConverters));
}
@Override
public boolean removeTypeConverter(Class<?> toType, Class<?> fromType) {
- return getDelegate().removeTypeConverter(toType, fromType);
+ boolean removed = getDelegate().removeTypeConverter(toType, fromType);
+ // replayed as well, so a rebuild reproduces the sequence rather than
resurrecting the converter
+ programmaticRegistrations.add(registry ->
registry.removeTypeConverter(toType, fromType));
+ return removed;
}
Review Comment:
Both fixed, by the keyed collection in the `programmaticRegistrations`
thread.
- **Appends an inverse instead of pruning** — `removeTypeConverter` now
deletes the matching `TypeConvertible` entry. Same end state on replay, and the
converter is actually released rather than staying reachable through the add
lambda.
- **Appends even when nothing was removed** — a removal for a pair that was
never registered now leaves the collection untouched. `removed` is still
returned to the caller, it just no longer drives a retained entry. Test:
`removingAConversionThatWasNeverRegisteredDoesNotAccumulate`.
The lost-registration window you noted here is closed by the same
`synchronized` as on `register()` — `removeTypeConverter` is synchronized too,
so its `getDelegate()` and its prune are one step.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -121,6 +169,8 @@ protected void doStart() throws Exception {
protected void doStop() throws Exception {
this.tracker.close();
this.trackerOpened = false;
+ // close() calls removedService for everything still tracked, this
only makes the end state explicit
+ this.trackedLoaders.clear();
ServiceHelper.stopService(this.delegate);
this.delegate = null;
}
Review Comment:
Fixed, and thank you for reproducing it — I had not thought about the
restart path at all.
`doStop()` now clears `programmaticRegistrations` alongside
`trackedLoaders`, inside a `synchronized (this)` block so it cannot race a
rebuild. The `tracker.close()` call stays outside the monitor, since that is a
framework call and putting it under the lock is the thing we spent the first
round removing.
Test added next to `removedServiceReleasesTheService` as you suggested:
`restartDoesNotReplayThePreviousLifecycle` runs `start()` -> `addTypeConverter`
-> `stop()` -> `start()` and asserts both that nothing is retained and that the
lookup comes back `null`. It fails against the previous head.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +84,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;
}
}
Review Comment:
Filed as **#744** rather than grown into this PR, as you suggested.
Good catch on the retry path — `ServiceTracker.open()` assigning `tracked`
before calling `trackInitial()` means the flag-based retry is not just useless
but actively misleading, since the second call returns as though it had
succeeded. Worth noting in the issue that the fix probably is not the flag: a
single loader throwing out of `trackInitial()` aborting the rest of the initial
set is the more interesting half.
The `synchronized` stays here for the reason in the thread above.
_Claude Code on behalf of Andrea Cosentino_
--
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]
