jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608067
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -173,27 +242,33 @@ 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(new TypeConvertible<>(fromType, toType),
+ registry -> registry.addTypeConverter(toType, fromType,
typeConverter));
}
@Override
public void addTypeConverters(Object typeConverters) {
- getDelegate().addTypeConverters(typeConverters);
+ register(typeConverters, registry ->
registry.addTypeConverters(typeConverters));
Review Comment:
**The keyed map only prunes the `addTypeConverter` / `addConverter` pair.
`addTypeConverters`, `addBulkTypeConverters` and `addFallbackTypeConverter` are
keyed by contributed-instance identity and have no removal API, so the map
still grows without bound and still pins bundle classloaders.**
`removeTypeConverter` is the only pruning path and it only deletes a
`TypeConvertible` key. `TypeConverterRegistry` exposes no
`removeTypeConverters` / `removeFallbackTypeConverter` /
`removeBulkTypeConverters`, so entries recorded here (and at the
`addBulkTypeConverters` / `addFallbackTypeConverter` calls below) live until
`doStop()`.
That breaks the stated rationale for keying on the affected path.
"Re-registering replaces rather than appends, which is what a Blueprint
container refresh does" is true for `addTypeConverter`, but a container refresh
instantiates a **new bean**, so `addTypeConverters(newBeanInstance)` hashes to
a fresh identity key and appends. 50 refreshes of a bundle contributing a
`TypeConverters` bean leave 50 entries, each strongly holding the bean and
through it the classloader of a bundle that may already be uninstalled — the
leak shape the review flagged, on the path the PR description names as its
motivating case. `addRemovePairsDoNotAccumulate` only exercises
`addTypeConverter` / `removeTypeConverter`.
Secondary point: all three share one untyped key space, and
`BulkTypeConverters extends Ordered, TypeConverter`, so the same instance
passed to both `addBulkTypeConverters` and `addFallbackTypeConverter` collides
and one registration is dropped from the replay.
(`CoreTypeConverterRegistry.addBulkTypeConverters` is also a bare `return` in
4.18, so recording that one is dead weight either way.)
--
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]
