jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608088
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -99,9 +138,21 @@ public Object
addingService(ServiceReference<TypeConverterLoader> serviceReferen
public void modifiedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
}
+ // not synchronized, for the same reason as addingService
Review Comment:
**This comment is anchored here, but the defect is `this.delegate = null` at
line 163 just below (and the `ServiceHelper.stopService(this.delegate)` above
it) — an unsynchronized write to a field `getDelegate()` now guards with a
monitor, so an invalidation can be swallowed by an in-flight rebuild.**
Interleaving:
1. T1 in synchronized `getDelegate()`: `delegate == null`, enters
`createRegistry()`, snapshots `trackedLoaders` (contains `sr_A`), begins
loading.
2. T2 (framework dispatch, unsynchronized `removedService` for `sr_A`):
removes `sr_A` from `trackedLoaders`, calls
`ServiceHelper.stopService(this.delegate)` on the still-null field, sets
`delegate = null`.
3. T1 finishes and assigns `delegate` = the registry it built **with**
`sr_A`'s converters.
The invalidation is lost and nothing will trigger another rebuild, so the
context permanently serves converters from a bundle that is gone. The
mirror-image interleaving hands a stopped delegate to a live caller.
I see this is split out to #743, which is reasonable as a scoping call — but
widening `getDelegate()` to a full monitor in *this* PR makes the window
larger, not smaller, so it is worth confirming the deferral still holds rather
than inheriting it from the pre-monitor shape.
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +97,36 @@ public OsgiTypeConverter(BundleContext bundleContext,
CamelContext camelContext,
this.tracker = new ServiceTracker<>(bundleContext,
TypeConverterLoader.class.getName(), this);
}
- private void ensureTrackerOpen() {
+ private synchronized void ensureTrackerOpen() {
Review Comment:
**`ensureTrackerOpen()` is now synchronized on `this`, but `doStop()` closes
the tracker and clears `trackerOpened` without that monitor — and
`getDelegate()` has no lifecycle guard, so the tracker can be left open after
stop, permanently.**
`BaseService.stop()` runs `doStop()` under its own `ReentrantLock`, not
under `this`, so the two are not mutually excluded. Interleaving: T1 in
`doStop()` executes `tracker.close()`; T2 inside synchronized `getDelegate()`
-> `ensureTrackerOpen()` calls `tracker.open()`; T1 then sets `trackerOpened =
false`. Result: a live, listening `ServiceTracker` on a `STOPPED` service,
holding a `getService()` use count for every `TypeConverterLoader`.
Nothing can clean that up — `BaseService.stop()` returns early on `if
(status == STOPPED || status == SHUTTING_DOWN || status == SHUTDOWN)`, so a
second `stop()` never reaches `doStop()`.
The same hole exists with no race at all: `getDelegate()` performs no
`isStopped()` / `isStopping()` check, so one stray conversion after stop calls
`ensureTrackerOpen()` -> `tracker.open()` and resurrects the tracker for the
life of the framework, pinning every contributing bundle's classloader.
Synchronizing one half of a two-field invariant is worse than neither, because
it reads as protected. `restartDoesNotReplayThePreviousLifecycle` shows the
lifecycle was considered, but only the explicit stop/start path.
--
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]
