jbonofre commented on code in PR #741:
URL: https://github.com/apache/camel-karaf/pull/741#discussion_r3907732586
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiFactoryFinder.java:
##########
@@ -79,23 +87,51 @@ public Optional<Class<?>> findClass(String key) {
// NOTE, the first found factory will be return
public BundleEntry getResource(String name) {
BundleEntry entry = null;
- Bundle[] bundles;
+ // only allocated when more than one bundle provides the same
descriptor, which is not the normal case
+ List<Bundle> alsoProviding = null;
- bundles = bundleContext.getBundles();
+ Bundle[] bundles = bundleContext.getBundles();
- URL url;
for (Bundle bundle : bundles) {
- url = bundle.getEntry(getResourcePath() + name);
+ URL url = bundle.getEntry(getResourcePath() + name);
Review Comment:
Addressed, and this is now the point of the PR. The scan skips
`Bundle.UNINSTALLED` and catches `IllegalStateException` from `getEntry`
(needed as well as the state check, for the bundle uninstalled between the two).
Confirmed against the 4.18.1 bytecode: the mapping function in
`addToClassMap` catches `Exception`, does `classesNotFoundExceptions.put(key,
e)` and rethrows, and on entry `addToClassMap` rethrows the cached exception
for that key. So yes — one race and the key is dead until `clear()`.
Two tests cover it: an `UNINSTALLED` bundle is never asked for an entry, and
a bundle that throws mid-scan does not fail the lookup for the bundles after it.
---
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiFactoryFinder.java:
##########
@@ -79,23 +87,51 @@ public Optional<Class<?>> findClass(String key) {
// NOTE, the first found factory will be return
public BundleEntry getResource(String name) {
BundleEntry entry = null;
- Bundle[] bundles;
+ // only allocated when more than one bundle provides the same
descriptor, which is not the normal case
+ List<Bundle> alsoProviding = null;
- bundles = bundleContext.getBundles();
+ Bundle[] bundles = bundleContext.getBundles();
- URL url;
for (Bundle bundle : bundles) {
Review Comment:
Agreed, the `break` is back. Selection and scan cost are unchanged from
`main`.
Worth recording why it matters more than it looks: misses cache too
(`classesNotFound`), so without the `break` every key — hit or miss — costs a
full sweep of all installed bundles, ~300 in a typical Camel Karaf install.
---
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiFactoryFinder.java:
##########
@@ -79,23 +87,51 @@ public Optional<Class<?>> findClass(String key) {
// NOTE, the first found factory will be return
public BundleEntry getResource(String name) {
BundleEntry entry = null;
- Bundle[] bundles;
+ // only allocated when more than one bundle provides the same
descriptor, which is not the normal case
+ List<Bundle> alsoProviding = null;
- bundles = bundleContext.getBundles();
+ Bundle[] bundles = bundleContext.getBundles();
- URL url;
for (Bundle bundle : bundles) {
- url = bundle.getEntry(getResourcePath() + name);
+ URL url = bundle.getEntry(getResourcePath() + name);
if (url != null) {
- entry = new BundleEntry();
- entry.url = url;
- entry.bundle = bundle;
- break;
+ if (entry == null) {
+ entry = new BundleEntry();
+ entry.url = url;
+ entry.bundle = bundle;
+ } else {
+ if (alsoProviding == null) {
+ alsoProviding = new ArrayList<>();
+ }
+ alsoProviding.add(bundle);
+ }
}
}
+ if (alsoProviding != null) {
+ // the scan order is the container's bundle install order, so
which bundle wins is not something
+ // the operator chose. Say so rather than resolving silently:
during a rolling upgrade with two
+ // versions of a bundle installed side by side, this is how a
patched bundle gets ignored. The
+ // result is also cached per key by findClass, so the choice made
here is sticky.
+ LOG.warn("Factory descriptor {} is provided by more than one
bundle. Using the one from {},"
Review Comment:
Confirmed, and it is a guaranteed false positive rather than a possible one
— the two bundles ship *different* implementations:
- `camel-xml-io` → `class=org.apache.camel.xml.LwModelToXMLDumper`
- `camel-xml-jaxb` → `class=org.apache.camel.xml.jaxb.JaxbModelToXMLDumper`
and both are in the default `camel-core` feature (`camel-features.xml:285`
and `:288`). So a stock install would warn at every startup.
Dropped the WARN. Note the same ambiguity exists in flat-classpath Camel,
where classpath order picks the winner — it is upstream, not a Karaf artifact.
Trimming one of the two bundles from the `camel-core` feature is a separate
discussion; since they provide different dumpers it is a behaviour change, not
just a dedup.
---
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiFactoryFinder.java:
##########
@@ -79,23 +87,51 @@ public Optional<Class<?>> findClass(String key) {
// NOTE, the first found factory will be return
public BundleEntry getResource(String name) {
BundleEntry entry = null;
- Bundle[] bundles;
+ // only allocated when more than one bundle provides the same
descriptor, which is not the normal case
+ List<Bundle> alsoProviding = null;
- bundles = bundleContext.getBundles();
+ Bundle[] bundles = bundleContext.getBundles();
- URL url;
for (Bundle bundle : bundles) {
- url = bundle.getEntry(getResourcePath() + name);
+ URL url = bundle.getEntry(getResourcePath() + name);
if (url != null) {
- entry = new BundleEntry();
- entry.url = url;
- entry.bundle = bundle;
- break;
+ if (entry == null) {
+ entry = new BundleEntry();
+ entry.url = url;
+ entry.bundle = bundle;
+ } else {
+ if (alsoProviding == null) {
+ alsoProviding = new ArrayList<>();
+ }
+ alsoProviding.add(bundle);
+ }
}
}
+ if (alsoProviding != null) {
Review Comment:
You are right, and I have not tried to patch around it — no logging inside
`getResource` can see that sequence, because step 3 never reaches `getResource`
at all. Confirmed in the bytecode: `classMap.computeIfAbsent`, so a resolved
key never re-scans.
So the PR no longer claims to detect it. What it keeps is the cheap, honest
part: a DEBUG line naming the bundle that supplied each descriptor, which
answers "which one is actually in use" for a context that resolves after the
fact.
I have left #733 open for the sticky-cache half. Doing it properly means
invalidating on bundle events the way `OsgiTypeConverter` revalidates its
delegate — with the wrinkle that `OsgiFactoryFinder` has no dispose hook today
(it is created per resource path by `OsgiFactoryFinderResolver` and never torn
down), so a `BundleListener` registered from it would leak. That wants its own
PR.
---
_Claude Code 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]
