oscerd commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3853471973
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,15 +67,15 @@ public OsgiTypeConverter(BundleContext bundleContext,
CamelContext camelContext,
this.tracker = new ServiceTracker<>(bundleContext,
TypeConverterLoader.class.getName(), this);
}
- private void ensureTrackerOpen() {
+ private synchronized void ensureTrackerOpen() {
Review Comment:
Kept, but for a narrower reason than the original commit implied — you are
right that the activator's own serialisation covers the common path.
`ensureTrackerOpen()` is reachable from both `doStart()` and
`getDelegate()`, and `trackerOpened` is a check-then-act, so two threads
arriving together could both call `tracker.open()`. It is cheap insurance
rather than something the activator guarantees.
What changed is that it is now safe to hold: `addingService` /
`removedService` no longer take this monitor, so `tracker.open()` firing
`trackInitial()` synchronously from inside the lock cannot come back round and
block on it.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,15 +67,15 @@ 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;
}
}
@Override
- public Object addingService(ServiceReference<TypeConverterLoader>
serviceReference) {
+ public synchronized Object
addingService(ServiceReference<TypeConverterLoader> serviceReference) {
Review Comment:
Removed.
`addingService` and `removedService` are both unsynchronized again, and the
fix goes a step further than dropping the keyword: the loaders the tracker
hands us are now kept in a `ConcurrentHashMap<ServiceReference,
TypeConverterLoader>` field, maintained by the two callbacks.
`createRegistry()` iterates a sorted snapshot of that map instead of calling
`tracker.getServiceReferences()` / `tracker.getService()`, so there is no call
into the tracker or the framework left underneath this instance's monitor at
all.
Detail on the lock ordering itself in my reply on the `getDelegate()` thread.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -100,8 +100,17 @@ public void
modifiedService(ServiceReference<TypeConverterLoader> serviceReferen
}
@Override
- public void removedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
+ public synchronized void
removedService(ServiceReference<TypeConverterLoader> serviceReference, Object
o) {
Review Comment:
Removed, same change.
`removedService` now drops the reference from the tracked-loader map, ungets
the service and invalidates the delegate — no monitor held, and no framework
call under a lock.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -67,15 +67,15 @@ 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;
}
}
@Override
- public Object addingService(ServiceReference<TypeConverterLoader>
serviceReference) {
+ public synchronized Object
addingService(ServiceReference<TypeConverterLoader> serviceReference) {
LOG.trace("AddingService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
TypeConverterLoader loader =
bundleContext.getService(serviceReference);
if (loader != null) {
Review Comment:
Fixed. `addingService` now wraps the load in a try/catch, drops the
reference from the tracked-loader map and calls
`bundleContext.ungetService(serviceReference)` before re-throwing.
While in there: the normal path leaked too. `removedService` never ungot the
service either, so *every* tracked loader's use count stayed above zero for the
lifetime of the framework, not only the ones whose `load()` threw. Since
`addingService` is what takes the service — we are the customizer, so the
tracker does not take it for us — releasing it is ours to do on both paths, and
`removedService` now ungets as well. Flagging that explicitly rather than
folding it in silently.
Both leaks predate this PR: `git diff main...` shows `addingService`'s body
was byte-identical to `main` apart from the `synchronized` keyword.
Tests: `addingServiceReleasesTheServiceWhenLoadingFails` and
`removedServiceReleasesTheService`, both of which fail against the previous
commit.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -100,8 +100,17 @@ public void
modifiedService(ServiceReference<TypeConverterLoader> serviceReferen
}
@Override
- public void removedService(ServiceReference<TypeConverterLoader>
serviceReference, Object o) {
+ public synchronized void
removedService(ServiceReference<TypeConverterLoader> serviceReference, Object
o) {
LOG.trace("RemovedService: {}, Bundle: {}", serviceReference,
serviceReference.getBundle());
+ if (this.delegate != null) {
+ // the rebuild in createRegistry replays the core converters and
the loaders the tracker still
Review Comment:
Agreed — fixed properly rather than logged. I went with your first option,
the replay list.
`OsgiTypeConverter` now records every registration made through the facade,
in order, as a `CopyOnWriteArrayList<Consumer<TypeConverterRegistry>>`, and
`createRegistry()` replays it onto the rebuilt registry after the loaders have
been loaded. That covers `addTypeConverter`, `addTypeConverters`,
`addBulkTypeConverters`, `addFallbackTypeConverter` and `addConverter` — and
also `setInjector`, `setTypeConverterExists` and
`setTypeConverterExistsLoggingLevel`, which were being lost in exactly the same
way. `removeTypeConverter` is recorded too, so a rebuild reproduces the
sequence rather than resurrecting a converter that was deliberately taken out.
I did not take the partial-removal route: unloading only the departing
loader's converters needs a way to attribute a converter to its origin, which
`TypeConverterRegistry` does not expose, and it would undo the
invalidate-and-rebuild strategy that d54f9a806 (#625) introduced on purpose.
Happy to revisit it as a separate change if you would rather have that shape.
The warning stays, but now says what actually happens — the registry is
discarded and rebuilt on next use, with programmatic registrations replayed —
and it is suppressed while the context is stopping, so `tracker.close()` no
longer emits one per loader on shutdown.
Tests: `programmaticConverterSurvivesARegistryRebuild` and
`removedTypeConverterIsNotResurrectedByARebuild`.
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -242,15 +251,26 @@ public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
}
public DefaultTypeConverter getDelegate() {
- if (delegate == null) {
- // ensure the tracker is open so we can discover
TypeConverterLoader services
Review Comment:
Changed as asked — `getDelegate()` is now plainly `synchronized`, with no
double-checked read.
One thing I would rather be straight about than let the thread read as
closed: it does not actually fix the race you describe. The caller still
receives the delegate and uses it after the monitor is released, so
`removedService` can stop it a moment later just the same — always locking
narrows the window, it does not close it. The race is also older than this PR;
before it, `getDelegate()` read the field with no lock at all.
Closing it properly means not stopping the outgoing delegate eagerly: let
the old instance keep serving in-flight conversions and let it go when nothing
references it. That is a different change and I would rather not smuggle it
into this one. Shall I open an issue for it?
_Claude Code on behalf of Andrea Cosentino_
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -242,15 +251,26 @@ public void setTypeConverterExists(TypeConverterExists
typeConverterExists) {
}
public DefaultTypeConverter getDelegate() {
- if (delegate == null) {
- // ensure the tracker is open so we can discover
TypeConverterLoader services
- // before creating the registry - this is important because
getDelegate() may be
- // called during doInit() (e.g. when to() eagerly creates
endpoints) which happens
- // before doStart() where the tracker is normally opened
- ensureTrackerOpen();
- delegate = createRegistry();
+ DefaultTypeConverter answer = delegate;
+ if (answer == null) {
+ // double checked locking against the volatile field: getDelegate
is on the conversion hot path,
+ // so the common case must stay lock free, but the check and the
assignment together are not
+ // atomic - without the lock two threads racing on first access
each build a registry, and
+ // whatever was registered on the one that loses is silently
dropped
+ synchronized (this) {
+ answer = delegate;
+ if (answer == null) {
+ // ensure the tracker is open so we can discover
TypeConverterLoader services
+ // before creating the registry - this is important
because getDelegate() may be
+ // called during doInit() (e.g. when to() eagerly creates
endpoints) which happens
+ // before doStart() where the tracker is normally opened
+ ensureTrackerOpen();
+ answer = createRegistry();
Review Comment:
Fixed, and by removing the call rather than reordering it:
`createRegistry()` no longer touches the `ServiceTracker`. `addingService` /
`removedService` maintain a `ConcurrentHashMap<ServiceReference,
TypeConverterLoader>`, and the rebuild iterates a sorted snapshot of that, so
there is no `tracker.*` call inside `synchronized (this)` and no ordering to
get wrong.
One correction for the record, because I went and checked rather than taking
it on trust. I disassembled `org.osgi.util.tracker.AbstractTracked` 1.5.4:
- in `trackAdding`, `customizerAdding` is invoked at bytecode offset 8 —
**before** the first `monitorenter` at offset 16;
- in `untrack`, `customizerRemoved` is invoked at offset 78 — **after**
every `monitorexit`.
So the reference `ServiceTracker` deliberately does not hold the `Tracked`
monitor across customizer callbacks, and that specific A/B cycle could not have
formed.
That does not rescue the code as it stood, and the change stands: holding
our own monitor across `tracker.getService()` and `bundleContext.getService()`
is the wrong shape regardless, it made the class hostage to a `ServiceTracker`
implementation detail, and `synchronized addingService` also meant every
conversion in the container queued behind an arbitrary bundle's `load()` for as
long as it took. I just did not want to leave a deadlock in the commit history
that the implementation does not actually admit. Happy to be corrected if you
know of a framework that does hold `Tracked` there.
_Claude Code on behalf of Andrea Cosentino_
--
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]
