jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608121
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -203,7 +278,7 @@ public TypeConverter lookup(Class<?> toType, Class<?>
fromType) {
@Override
public void setInjector(Injector injector) {
Review Comment:
**`setInjector` only records a replay lambda and never updates the
`injector` field, so every rebuilt registry is constructed with the
constructor-time injector and loads its core converters before the replay
corrects it.**
`createRegistry()` passes the `final` field `this.injector` to `new
OsgiDefaultTypeConverter(...)`, then calls `answer.init()` and
`answer.loadCoreAndFastTypeConverters()`, and only afterwards calls
`replayProgrammaticRegistrations(answer)`. So after a rebuild, converters
instantiated during core loading used the **old** injector; the configured one
is swapped in only after the fact, and anything already created keeps the stale
reference.
`TYPE_CONVERTER_EXISTS_KEY` and `TYPE_CONVERTER_EXISTS_LOGGING_LEVEL_KEY`
have the same ordering problem, and it compounds the replay issue on
`replayProgrammaticRegistrations`: a context configured `Override` or `Fail`
has all its core and loader converters registered under the constructor
defaults (`Ignore` / `DEBUG`), and the real policy only takes effect partway
through the replay, in insertion order.
This is strictly better than pre-PR behaviour (where the setting was lost
entirely), so not a blocker — but "brought back to the same state" is not what
happens.
_AI-generated review on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -279,28 +358,73 @@ 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.values()) {
+ registration.accept(registry);
+ }
+ }
+
+ /**
Review Comment:
**Two consecutive Javadoc blocks: this one is orphaned by the `/** */` for
`programmaticRegistrationCount()` that follows, so `register()` ends up with no
Javadoc and the surviving comment documents the wrong method.**
Java attaches only the last block before a declaration. This block — the
load-bearing explanation of why apply-and-record has to be atomic, which both
the commit message and the PR description lean on — becomes a dangling comment
attached to nothing, and `register()` at line 421 is undocumented. The `{@link
#getDelegate()}` reference in it is invisible to doclint too.
Looks like a rebase/squash artifact; moving this block down to immediately
precede `register()` fixes it.
_AI-generated review on behalf of JB Onofré_
--
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]
