jbonofre commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4027297881
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -364,15 +371,17 @@ protected void trackService(String className, String
filter) throws InvalidSynta
* @return The actual tracker service object.
*/
protected <T> T getTrackedService(Class<T> clazz) {
- SingleServiceTracker tracker = trackers.get(clazz.getName());
+ @SuppressWarnings("unchecked")
Review Comment:
The runtime type check on the returned service was dropped. Previously
`clazz.cast(tracker.getService())` threw an immediate, clearly-attributed
`ClassCastException` if the tracked object wasn't actually assignable to
`clazz` (e.g. duplicate class definitions across bundle classloaders). Now it's
an unchecked cast on the tracker itself, and `tracker.getService()` is returned
with no check: a type mismatch surfaces later (if at all) as a confusing
`ClassCastException` far from the real cause, or not at all if passed through
untyped code.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -320,40 +328,39 @@ public void run() {
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct.
*/
- protected void trackService(Class<?> clazz) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
- SingleServiceTracker tracker = new
SingleServiceTracker<>(bundleContext, clazz, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(clazz.getName(), tracker);
- }
+ protected <T> void trackService(Class<T> clazz) throws
InvalidSyntaxException {
+ trackService(clazz, null);
}
/**
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
* @param filter The filter to use to select the services to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct (in
the filter especially).
*/
- protected void trackService(Class<?> clazz, String filter) throws
InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
+ protected <T> void trackService(Class<T> clazz, String filter) throws
InvalidSyntaxException {
+ if (!trackers.containsKey(clazz)) {
if (filter != null && filter.isEmpty()) {
filter = null;
}
- SingleServiceTracker tracker = new
SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
+ SingleServiceTracker<T> tracker = new
SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
tracker.open();
- trackers.put(clazz.getName(), tracker);
+ trackers.put(clazz, tracker);
}
}
protected void trackService(String className, String filter) throws
InvalidSyntaxException {
- if (!trackers.containsKey(className)) {
- SingleServiceTracker tracker = new
SingleServiceTracker<>(bundleContext, className, filter, (u, v) ->
reconfigure());
- tracker.open();
- trackers.put(className, tracker);
- }
+ try {
+ Class<?> clazz = Class.forName(className);
Review Comment:
This changes `trackService(String, String)` from pure name-based tracking to
eager class resolution via `Class.forName`.
I have two concerns here:
1. This method is `protected`. A subclass calling it with a class name not
importable by its own bundle will now get a silently-dropped tracker (just a
WARN log) instead of the name-based OSGi filter tracking this overload exists
to provide.
2. Only `ClassNotFoundException` is caught. A bad static initializer or a
missing transitively-referenced class raises
`ExceptionInInitiliazerError`/`NoClassDefFoundError`, which isn't caught here
and will propagate out of `doOpen()`/`start(BundleContext)`, aborting the whole
bundle's activation. The old code could never throw from this path.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -86,8 +96,9 @@ public void start(BundleContext context) throws Exception {
doOpen();
scheduled.set(false);
if (managedServiceRegistration == null
- && trackers.values().stream()
- .allMatch(t -> t.getService() != null)) {
+ && trackers.values().stream()
+ .map(SingleServiceTracker::getService)
+ .allMatch(Objects::nonNull)) {
Review Comment:
`allMatch(...)` on `trackers.values()` is vacuously `true` when `trackers`
is empty. If a required service's class fails to resolve it
`trackService(String, String)`, it's never added to `trackers` at all, so this
check now incorrectly reports "ready" instead of waiting.
`doStart()` then calls `getTrackedServices(RequiredInterface.class)`, which
throws `IllegalStateException("Service not tracked for class ...)`: a confusing
crash disconnected from the real (silently logged) root cause.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -35,29 +37,37 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
-
-import org.osgi.framework.*;
+import org.osgi.framework.BundleActivator;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.cm.Configuration;
import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ManagedService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class BaseActivator implements BundleActivator, Runnable, ThreadFactory
{
+public class BaseActivator implements BundleActivator, ManagedService,
Runnable, ThreadFactory {
Review Comment:
Making `BaseActivator` directly `implement ManagedService` turns
`org.osgi.service.cm` into a hard compile/link-time dependency for every bundle
embedding this class, not just ones that call `manage(pid)`.
The ~ 20 pom.xml additions in this PR cover in-tree callers, but this is a
compatibility-breaking change for any downstream/third-party Karaf-based budle
that extends `BaseActivator` (including Karaf subprojects like Cellar or
Decanter) without Configuration Admin on its classpath: it would now fail to
load the class at all (`NoClassDefFoundError`), even if it never calls
`manage()`.
I think it's worth calling out explicitly since `BaseActivator` is public
API.
--
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]
