jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3860502109
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -238,10 +291,13 @@ public TypeConverterExists getTypeConverterExists() {
@Override
public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
- getDelegate().setTypeConverterExists(typeConverterExists);
+ register(registry ->
registry.setTypeConverterExists(typeConverterExists));
}
- public DefaultTypeConverter getDelegate() {
+ // fully synchronized rather than double checked: the delegate is not
immutable after publication -
+ // removedService stops and replaces it - so a lock free read of the field
buys a race for no real gain,
+ // conversion work dwarfing an uncontended monitor either way
+ public synchronized DefaultTypeConverter getDelegate() {
Review Comment:
Not blocking, but the invariant the safety argument rests on does not hold,
so I would like the comment and the PR description to stop asserting it.
The PR body says there is "no call into the tracker or the framework left
underneath this instance's monitor at all". There are several. `getDelegate()`
is now `synchronized` and calls `ensureTrackerOpen()`, and `tracker.open()`
does `bundleContext.addServiceListener(...)` and
`bundleContext.getServiceReferences(...)`, then synchronously runs
`trackInitial()` -> your own `addingService` -> `bundleContext.getService(...)`
— all under `this`. Separately, `createRegistry()` runs foreign `loader.load()`
under `this`.
No ABBA results, because the callbacks no longer take this monitor, and (per
your disassembly, which I verified) `Tracked` is not held across the customizer
either. So the code is *not* deadlock-prone. But "we removed the `tracker.*`
calls from `createRegistry()`" and "nothing framework-facing runs under our
monitor" are different claims, and only the first one is true. Holding `this`
across arbitrary bundle `load()` is the same shape I objected to originally; it
is now reached by a different route.
Second, on this comment specifically: "conversion work dwarfing an
uncontended monitor" is true in steady state and misleading during a rebuild,
which is the case that matters. While `createRegistry()` runs — core converter
package scan plus every tracked loader's `load()` — every conversion in the
container is blocked on this monitor, and per the `programmaticRegistrations`
thread that duration grows over the life of the context.
I am fine with the tradeoff. Please just say what it is: all conversions
serialize behind a rebuild, and the monitor is held across foreign bundle code.
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -203,7 +256,7 @@ public TypeConverter lookup(Class<?> toType, Class<?>
fromType) {
@Override
public void setInjector(Injector injector) {
- getDelegate().setInjector(injector);
+ register(registry -> registry.setInjector(injector));
}
Review Comment:
Minor: `setInjector` / `setTypeConverterExists` /
`setTypeConverterExistsLoggingLevel` are *state*, not events, and replaying
them as ordered lambdas gets the ordering subtly wrong.
`createRegistry()` constructs `OsgiDefaultTypeConverter` with the
constructor-time `injector` field, runs `init()` and
`loadCoreAndFastTypeConverters()`, and only then replays. So a rebuilt registry
loads its core converters under the *old* injector and swaps afterwards — not
equivalent to a registry built fresh with the current one. `this.injector` is
never updated by this setter either.
Still an improvement on losing them entirely, so not blocking. But holding
them as plain fields and applying them at construction would be both simpler
and correct, and would keep three entries out of the replay list.
_Claude Code on behalf of JB Onofré_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,27 +84,36 @@ public OsgiTypeConverter(BundleContext bundleContext,
CamelContext camelContext,
this.tracker = new ServiceTracker<>(bundleContext,
TypeConverterLoader.class.getName(), this);
}
- private void ensureTrackerOpen() {
+ private synchronized void ensureTrackerOpen() {
if (!trackerOpened) {
tracker.open();
trackerOpened = true;
}
}
Review Comment:
Agreed on keeping this, and your narrower justification is the right one.
One nit while you are in here, pre-existing but now more reachable: if
`tracker.open()` throws, `trackerOpened` stays `false`, but
`ServiceTracker.open()` assigns `tracked` *before* calling `trackInitial()`, so
the retry returns early and never re-runs `trackInitial()`. The initial loaders
that had not been processed yet are then never tracked at all.
`addingService` can throw `RuntimeCamelException` out of `trackInitial()`
when a loader's `load()` fails, so one bad loader at startup can silently
strand the others. Worth a separate issue rather than growing this PR —
flagging it because the method is in the diff.
--
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]
