jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3860502103
##########
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:
**Blocking**, and a one-liner.
`trackedLoaders` is cleared here but `programmaticRegistrations` is not, so
a stop/start cycle replays registrations from the previous lifecycle —
including converters owned by bundles that are gone by the time the context
comes back up.
Reproduced against this head: `start()` -> `addTypeConverter(String, Marker,
tc)` -> `stop()` -> `start()` -> `getDelegate().lookup(String.class,
Marker.class)` returns the converter from the previous lifecycle.
`ServiceSupport` permits stop/start, and `OsgiTypeConverter` is a
context-scoped service, so this is reachable on a context restart. Clearing the
list alongside `trackedLoaders` is enough, and it would be worth a test next to
`removedServiceReleasesTheService`.
##########
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:
Two things here, both feeding the retention problem in the
`programmaticRegistrations` thread.
**Appends an inverse instead of pruning.** Recording a *removal* rather than
deleting the matching *add* means the removed converter stays strongly
reachable forever — the add lambda still holds it. Replay reproduces the right
end state, so the tests pass, but the object is never released. Deleting the
entry would give the same end state and actually let go.
**Appends even when nothing was removed.** `removed` is computed and
returned but not consulted. A `removeTypeConverter` for a pair that was never
registered still grows the list and still costs a no-op call on every future
rebuild.
Also, as on the `register()` thread: the `getDelegate()` call and the `add`
are not atomic with respect to each other, so this has the same
lost-registration window.
--
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]
