jbonofre commented on code in PR #741:
URL: https://github.com/apache/camel-karaf/pull/741#discussion_r3878287707
##########
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:
Dropping the `break` makes scan on all bundles. For performance reason, I
would keep it.
##########
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:
`bundleContext.getBundles()` returns a snapshot, and per the OSGi spec
`Bundle.getEntry()` throws `IllegalStateException` if this bundle has been
uninstalled. Before this change, the loop broke at the winner, so only bundles
ahead of it were called, and since the descriptors live in early-installed
bundles, that was typically a handful.
Now every one of the ~300 installed bundles is called on every key
resolution, so the exposure widens by about two orders of magnitude.
What turns that from a transient error into a lasting one is the caching in
Camel. `DefaultFactoryFinder.addToClassMap`:
```java
} catch (Exception e) {
classesNotFoundExceptions.put(key, e);
throw RuntimeCamelException.wrapRuntimeException(e);
}
```
and on entry it rethrows the cached exception for that key.
`IllegalStateException` is an `Exception`, and `getResource` is called from
inside this lambda (see `findClass`, line 50). So a `feature:uninstall` or
`bundle:update` racing a resolution, including the rolling upgrade this PR is
about (leaves that key throwing for the life of the context, until `clear()`).
I think we should skip bundles in state `Bundle.UNINSTALLED` and wrap the
`getEntry` call in a `try`/`catch (IllegalStateException)` and continue (or
keep the `break` resolution and do the duplicate detection in a separate pass
that cannot fail the lookup).
##########
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:
The default `camel-core` feature installs both `camel-xml-io` and
`camel-xml-jaxb`, and both bundles ship
`META-INF/services/org/apache/camel/modelxml-dumper` with different
implementation classes.
It's not a big deal in OSGi as we control the loading, so not sure this warn
is useful. We could also fix the duplication in the `camel-core` feature (we
don't need the two bundles).
##########
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:
#733 is about a rolling upgrade: a patched bundle is installed on top on the
old one and silently ignored.
That sequence never reaches this branch here:
1. `camel-foo/x.y.z` installed, context starts, `findClass("some-factory")`
scans, find one provider, caches the class in the `classMap`: no ambiguity, no
WARN.
2. Users installs `camel-foo/x.y.f`
3. The running context keep using x.y.z class. `addToClassMap`
short-circuits on `classMap`, so `getResource` is never called again for that
key. And the finder that is actually ignoring the patched bundle never warns.
The WARN only appears in contexts create after both bundles are installed.
The same blind spot applies during Karaf startup, where features install
progressively and an early-starting context can scan before all providers exist.
So the stated goal ("it is what tells us whether the ambiguity happens in
practice") is not met for the #733 scenario. Catching it needs a
`BundleListener` or cache invalidation on bundle events, in the spirit of how
`OsgiTypeConverter` in validates its delegate on service changes.
--
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]
