Fixed various IntelliJ warnings in BaseActivator - #2911
CptBartender wants to merge 1 commit into
Conversation
Added type parameters to generics Changed the class so that it explicitly implements ManagedService instead of sneakily registering itself as such Extracted the string splitting pattern to a static field so that the regex is compiled only once Changed getInterfaceNames so that it returns distinct classes in case any implemented interfaces extend the same superinterface Changed trackers map to use Class objects as keys instead of String names
Test Results 726 files ±0 726 suites ±0 1h 14m 31s ⏱️ + 2m 35s For more details on these errors, see this check. Results for commit 5320076. ± Comparison against base commit b4831ef. |
| trackers.put(className, tracker); | ||
| } | ||
| try { | ||
| Class<?> clazz = Class.forName(className); |
There was a problem hiding this comment.
This changes trackService(String, String) from pure name-based tracking to eager class resolution via Class.forName.
I have two concerns here:
- 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. - Only
ClassNotFoundExceptionis caught. A bad static initializer or a missing transitively-referenced class raisesExceptionInInitiliazerError/NoClassDefFoundError, which isn't caught here and will propagate out ofdoOpen()/start(BundleContext), aborting the whole bundle's activation. The old code could never throw from this path.
| .allMatch(t -> t.getService() != null)) { | ||
| && trackers.values().stream() | ||
| .map(SingleServiceTracker::getService) | ||
| .allMatch(Objects::nonNull)) { |
There was a problem hiding this 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.
| */ | ||
| protected <T> T getTrackedService(Class<T> clazz) { | ||
| SingleServiceTracker tracker = trackers.get(clazz.getName()); | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this 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.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class BaseActivator implements BundleActivator, Runnable, ThreadFactory { | ||
| public class BaseActivator implements BundleActivator, ManagedService, Runnable, ThreadFactory { |
There was a problem hiding this 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.
2 participants
instanceofcan be used - eventually it'll be bumped in Lift minimum JDK version to 21 and remove SecurityManager #2214, I presume