fix(#734): build the type converter registry once, and say when it is discarded

[oscerd]

Fixes #734

Rewritten after two review rounds, so this describes where the PR stands rather
than where it started.

1. Nothing calls the tracker or the framework under our monitor

The first attempt made addingService / removedService synchronized.
@jbonofre flagged the lock ordering, and the shape was wrong even where the
cycle does not close: createRegistry() called tracker.getServiceReferences()
and tracker.getService() while holding this, and a synchronized addingService meant every conversion in the container queued behind an
arbitrary bundle's load().

Both callbacks are unsynchronized again, and createRegistry() no longer
touches the ServiceTracker. The callbacks maintain a
ConcurrentHashMap<ServiceReference, TypeConverterLoader>, and the rebuild
iterates a sorted snapshot of it -- same service-ranking order as before, with
the high-ranking fallback converter installed last.

2. getDelegate() builds the registry exactly once

delegate being volatile makes the read safe but not the check-and-assign, so
two threads racing on first access each built a registry and one was silently
discarded.

Now plainly synchronized, no double-checked read. This narrows but does not
close the separate race where a delegate already handed to a caller is stopped
by removedService -- see the split-out section.

3. Programmatic converters survive the rebuild

The original PR only made the loss visible, which is what #734 asked for. Review
asked for it to be fixed, so it is.

Every registration made through this facade is recorded and replayed onto the
rebuilt registry: addTypeConverter, addTypeConverters,
addBulkTypeConverters, addFallbackTypeConverter, addConverter,
removeTypeConverter, plus setInjector / setTypeConverterExists /
setTypeConverterExistsLoggingLevel, which were being lost the same way.

The store is a keyed, insertion-ordered map, not a list (second review). The
key is what the registration is about -- a TypeConvertible for a converter,
the contributed instance for the bulk / fallback / addTypeConverters forms, a
sentinel per setter. Re-registering replaces rather than appends, which is what
a Blueprint container refresh does, and removeTypeConverter deletes the
matching entry rather than appending an inverse. An append-only list retained
every converter it had ever seen, and with it the classloader of the
contributing bundle: 500 add/remove pairs left 1000 entries. They leave none now.

Applying a registration and recording it is one step -- register() and
removeTypeConverter() are synchronized on the monitor getDelegate() takes.
A rebuild landing between the halves would replay a map the registration was not
in yet, then hand back a registry it had never been applied to.

doStop() clears the map. ServiceSupport permits stop then start and this is a
context-scoped service, so a restart would otherwise replay the previous
lifecycle's registrations, including converters owned by bundles that are gone.

Partial removal was the alternative. It needs a way to attribute a converter to
the loader that contributed it, which TypeConverterRegistry does not expose,
and it would undo the invalidate-and-rebuild strategy d54f9a806 (#625)
introduced deliberately.

The WARN stays but now describes what actually happens, and is suppressed while
the context is stopping so tracker.close() no longer logs one line per loader
on shutdown.

4. The service use count is released

addingService takes the service -- we are the customizer, so the tracker does
not take it for us -- which makes releasing it ours to do:

• failure path: the tracker treats a customizer that throws as never
  tracked, so removedService is never called for that reference and the use
  count was stuck above zero for the life of the framework. It now ungets before
  re-throwing.
• normal path: removedService never ungot either, so this leaked for
  every tracked loader. It ungets now.

Both predate this PR but sit in the methods under review.

What is deliberately not changed

addingService still loads into the existing delegate instead of invalidating
it. d54f9a806 (#625, PR #684) specifically reverted invalidate-on-add for that
reason.

Split out, not fixed here

• OsgiTypeConverter: delegate invalidation and eager stop happen outside the monitor #743 -- removedService writes delegate outside the monitor, so an
  invalidation can be swallowed by a rebuild already in flight; plus the eager
  stop of a delegate callers already hold. Same shape, one issue, at the
  reviewer's suggestion.
• OsgiTypeConverter: a failing tracker.open() permanently strands the remaining initial TypeConverterLoaders #744 -- a failing tracker.open() permanently strands the remaining
  initial TypeConverterLoaders.

Tests

16 cases in OsgiTypeConverterTest, eleven new across the two rounds:

test	guards
rebuiltRegistryReloadsTheTrackedLoadersWithoutAskingTheTracker	the rebuild uses the recorded loaders, not the tracker
programmaticConverterSurvivesARegistryRebuild	replay of programmatic registrations
removedTypeConverterIsNotResurrectedByARebuild	replay reproduces the sequence, removals included
addingServiceReleasesTheServiceWhenLoadingFails	unget on the failure path
removedServiceReleasesTheService	unget on the normal path
addingServiceMustNotHoldTheInstanceMonitor	forward guard on the callback locking
addRemovePairsDoNotAccumulate	500 add/remove pairs retain nothing (1000 before)
reRegisteringTheSameConversionReplaces	Blueprint refresh replaces, does not append
removingAConversionThatWasNeverRegisteredDoesNotAccumulate	no-op removals are not retained
restartDoesNotReplayThePreviousLifecycle	stop/start does not resurrect dead bundles' converters
registrationCannotInterleaveWithARebuild	forward guard on register() atomicity

Four of the five second-round tests fail against the previous commit,
addRemovePairsDoNotAccumulate at exactly the 1000 entries the review reported.
Both addingServiceMustNotHoldTheInstanceMonitor and
registrationCannotInterleaveWithARebuild pass against their predecessors too,
so they are forward guards rather than regression tests, and are labelled as
such rather than counted as coverage they do not provide.

Tests run: 16, Failures: 0, Errors: 0, Skipped: 0   (OsgiTypeConverterTest)
Tests run: 24, Failures: 0, Errors: 0, Skipped: 0   (camel-core-osgi)

Full-reactor mvn clean install -DskipTests: BUILD SUCCESS in 11:40.

---

Claude Code on behalf of Andrea Cosentino

[jbonofre]

I'm not sure it's actually required as the bundle activator is itself sync.
It's not a big deal to add this, but not sure it will actually do much.

[oscerd]

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

[jbonofre]

We should not use synchronized here: createRegistry() calls tracker.getServiceReference() and tracker.getService() while holding this, but the ServiceTracker holds its own Tracked monitor when it calls addingService()/removedService().

We should either not use synchronized here or snapshot the tracked services before entering the lock.

[oscerd]

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

[jbonofre]

Same here, synchronized should not be used on removedService(), else we can introduce a deadlock.

[oscerd]

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

[jbonofre]

bundleContext.getService() increments the framework use-count for this service reference. If loader.load(delegate) subsequently throws, addingService() propagates RuntimeCamelException. The ServiceTracker treats a throwing addingService() as "not tracked", so removedService() is never called for this reference and bundleContext.ungetService() is never invoked.

The use-count is permanently stuck above zero; the originating bundle cannot be cleanly uninstalled for the lifetime of the OSGi framework. Fix: wrap loader.load() in a try-catch and call bundleContext.ungetService(serviceReference) in the catch before re-throwing.

[oscerd]

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

[jbonofre]

When removedService() stops the entire delegate and createRegistry() rebuilds it, only converters discovered via the ServiceTracker are replayed. Converters registered programmatically via addTypeConverter(), addBulkTypeConverters(), or addFallbackTypeConverter(), including Blueprint bean-registered converters, are silently dropped with the old delegate.

Routes relying on those converters will fail with NoTypeConversionAvailableException after any bundle uninstalls a TypeConverterLoader, with no diagnostic pointing to the root cause. The log warning is insufficient. Either:

• Maintain a separate replay list of programmatically registered converters and re-apply them in createRegistry().
• Abandon full-delegate teardown in favour of partial removal (unloading only the converters contributed by the departing loader).

[oscerd]

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 d54f9a8 (#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

[jbonofre]

The fast path reads answer = delegate (volatile) and, if non-null, skips the synchronized block and returns answer directly to the caller. However, a concurrent removedService() (holding synchronized(this)) can call ServiceHelper.stopService(this.delegate) and set this.delegate = null after Thread A's null-check but before Thread A uses the returned value. Thread A then calls conversion methods on a stopped converter, resulting in IllegalStateException or silent data loss.

DCL is only safe when the published object is truly immutable after publication. A delegate that can be stopped and replaced does not meet that bar. Consider always going through the lock in getDelegate() — type conversion is not so hot that the lock overhead is measurable compared to actual conversion work.

[oscerd]

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

[jbonofre]

As mentioned before:

createRegistry() is called here while holding synchronized(this). Inside createRegistry(), it calls tracker.getServiceReferences() and tracker.getService(), which the Felix/Equinox ServiceTracker guards with its own internal Tracked monitor.

Meanwhile, the OSGi framework holds that same Tracked monitor when dispatching service events — it calls addingService() / removedService(), which this PR has made synchronized(this).

Thread A: holds synchronized(this) → calls tracker.getService() → waits for Tracked monitor
Thread B: holds Tracked monitor → calls addingService() → waits for synchronized(this)

Classic ABBA deadlock. This lock order did not exist before this PR. Fix: snapshot the tracked services outside any lock (e.g. call tracker.getTracked() which returns a defensive copy, or move the tracker.* calls before entering synchronized(this)), then use the snapshot inside the lock.

[oscerd]

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

[oscerd]

@jbonofre thanks — all seven threads are answered in 2c0676e26, and the PR description has been rewritten to match where the change now stands rather than where it started.

Summary of what moved:

• Callback locking — synchronized is off addingService / removedService, and createRegistry() no longer calls the ServiceTracker at all: the callbacks maintain a ConcurrentHashMap<ServiceReference, TypeConverterLoader> and the rebuild iterates a sorted snapshot of it, so nothing calls into the tracker or the framework under this instance's monitor.
• getDelegate() — plainly synchronized, no double-checked read.
• Use count — ungets on the failure path, and on the normal removedService path, which leaked for every tracked loader rather than only the throwing ones.
• Converter loss — fixed rather than logged, via the replay list you suggested.

Two of the replies push back a little rather than just saying "done", so they are worth a look before you re-review: I could not reproduce the Tracked lock inversion against the reference ServiceTracker bytecode (the fix still stands, for other reasons), and always-locking getDelegate() does not actually close the stop-after-return race — I would rather say that than let the thread read as resolved. Details in the two threads.

I have left all seven unresolved for you to close.

CI: green on 2c0676e26. The first run had one failure, CamelQuartzITest.testResultMock — Expected: <1> but was: <2> — which is a pre-existing flake rather than anything in this change: the route's cron is 0/1+*+*+*+*+?, one trigger per second, against a mock asserting exactly one message, and Java 21 passed on the same commit while Java 17 failed. A re-run of the failed job went green with no code change. Happy to open a separate issue for that test.

Claude Code on behalf of Andrea Cosentino

[jbonofre]

Thanks for the rewrite — the direction is right and the reasoning is a lot easier to follow than the first pass.

Two things before the findings.

Your AbstractTracked correction is accurate, and I was wrong. I disassembled org.osgi.util.tracker 1.5.4 independently rather than take it on trust: in ServiceTracker.open(), trackInitial() is invoked at offset 209, after every monitorexit; trackAdding calls customizerAdding before its first monitorenter. The specific A/B cycle I described could not form. I appreciate you checking instead of just complying.

Both ungetService fixes are real and correctly reasoned. When a ServiceTrackerCustomizer is supplied, ServiceTracker does not call getService/ungetService itself — the customizer owns both. Catching the normal-path leak (every tracked loader, not only the throwing ones) went beyond what was asked. Good catch.

On method: I built the branch and confirmed 11/11 tests pass, then wrote four throwaway probe tests against this head. All four reproduced, so the blocking findings below are confirmed by execution rather than read off the diff. Details in each thread.

There are four blocking items. Items 1 and 4 are small and mechanical; item 3 needs a design decision; item 2 I would rather split out — see that thread.

[jbonofre]

Blocking. This reintroduces the bug the PR fixes, one level down.

register() is apply-then-record, and no monitor spans both halves. A rebuild landing between them loses the registration from the live registry even though it is sitting in the replay list:

A: addTypeConverter -> getDelegate() returns D1 -> stalls inside the apply
B: removedService   -> delegate = null
C: getDelegate()    -> builds D2, replays the list (r is not in it yet)
A: programmaticRegistrations.add(r)
=> r is in the replay list, but was only ever applied to the discarded D1

I reproduced this against this head with a registry whose addTypeConverter I could stall inside: after the sequence above, d2.lookup(String.class, Marker.class) returns null, and no further rebuild happens to correct it. So the converter is silently absent from the live registry — exactly the symptom programmaticConverterSurvivesARegistryRebuild is meant to rule out.

The comment here is right that the registration cannot be applied twice; the gap is that it can be applied zero times.

The window is narrow for addTypeConverter, but addTypeConverters(Object) does reflective @Converter scanning, and Blueprint bean registration runs concurrently with bundle lifecycle events, so it is not theoretical.

Fix is cheap, since getDelegate() is reentrant on the same monitor:

private synchronized void register(Consumer<TypeConverterRegistry> registration) {

removeTypeConverter has the same split and needs the same treatment.

[oscerd]

Fixed exactly as you suggested — register() is private synchronized, and removeTypeConverter() has the same treatment.

You are right that the comment defended the wrong half. It argued the registration cannot be applied twice; the hole was that it can be applied zero times. I have rewritten it to say what the monitor is actually for.

On testing it: registrationCannotInterleaveWithARebuild stalls inside an overridden createRegistry() and asserts a concurrent addTypeConverter cannot proceed, then that it lands on the live registry. Being straight about its limits — it passes against the previous head too, because the old register() still went through a synchronized getDelegate() and blocked there anyway. So it is a forward guard, not a regression test. Reproducing the real interleave needs a registry that can be stalled inside its own addTypeConverter, the way your throwaway did; I did not build one, and said so in the commit message rather than let the test imply more coverage than it has.

Claude Code on behalf of Andrea Cosentino

[jbonofre]

Blocking, though I would rather see it split out than folded in here — see the bottom.

This write is not under the monitor that getDelegate() now takes, so making getDelegate() synchronized buys mutual exclusion between readers only, never against the writer. The invalidation can be swallowed entirely:

A: getDelegate() takes `this`, delegate == null, enters createRegistry()
   (loader L already loaded into the new registry)
B: removedService(L) -> stopService(this.delegate)  // still null, no-op
                     -> this.delegate = null        // no monitor held
A: delegate = <registry built with L in it>

I reproduced this by stalling at the end of createRegistry(): after removedService returns, getDelegate() hands back the same instance that was built with the removed loader in it. Assertion assertSame(built, afterRemoval) passes. So converters belonging to an unregistered bundle keep serving indefinitely, and no subsequent event will invalidate them.

This predates the PR — the old unsynchronized if (delegate == null) had the same lost write. But it sits squarely inside #734's point 1, and synchronized getDelegate() makes the window wider, not narrower: the monitor is now held across the full rebuild (core converter scan plus every loader.load()), so there is much more time for a removedService to land inside it.

An epoch counter would close it — read a generation before createRegistry(), re-check it before the assign, discard and retry if removedService bumped it meanwhile.

That said: this is the same underlying problem as the eager-stop race you raised on the getDelegate() thread — invalidation and stop both happening outside the monitor, to an instance callers already hold. I would rather have one follow-up issue covering both than see either bolted on here. Your call, but if you open that issue, put this in it and I will not hold the PR on it.

[oscerd]

Split out as you asked: #743, which covers both this lost invalidation and the eager-stop race from the getDelegate() thread, since they are the same shape — mutation of an instance callers already hold, performed outside the monitor that publishes it — and a fix for one constrains the fix for the other.

Your epoch-counter sketch is in there, along with your reproduction (stalling at the end of createRegistry(), assertSame(built, afterRemoval) passing) and the point that #739 widens the window rather than narrowing it, since the monitor is now held across the whole rebuild.

Nothing about it is fixed in this PR.

Claude Code on behalf of Andrea Cosentino

[jbonofre]

Reviewed the three commits on fix/734-typeconverter-rebuild (OsgiTypeConverter.java +143/-28, OsgiTypeConverterTest.java +153). The core idea — record programmatic registrations and replay them onto a rebuilt registry — is sound, and the two ungetService fixes and the trackedLoaders map are clear improvements. 14 inline comments below; three of them I would treat as merge blockers, all stemming from the replay being appended at the end of createRegistry() without accounting for how CoreTypeConverterRegistry actually behaves. I verified these against the camel-base-4.18.1 and camel-api-4.18 bytecode rather than from memory:

1. The unguarded replay can permanently brick the registry. addConverter() is a bare converters.put(), but addTypeConverter() goes through onTypeConverterExists(), which throws TypeConverterExistsException for TypeConverterExists.Fail and silently drops for the default Ignore (typeConverterExistsLoggingLevel defaults to DEBUG). Since loaders now load before the replay, a pair that did not collide when it was first registered does collide on rebuild. replayProgrammaticRegistrations is outside any try/catch, so the throw escapes getDelegate(), delegate stays null, and every conversion in the container fails from then on — with the offending entry still in the map, so retries fail identically.
2. !isStopped() suppresses the #734 warning in every pre-STARTED state. BaseService.isStopped() is status < STARTING || status >= STOPPED, and NEW/BUILT/INITIALIZING/INITIALIZED are all < STARTING. getDelegate()'s own comment says it may be called during doInit(), so in exactly that window the registry is discarded with no log line — the invisibility this PR exists to remove. isStopping() alone already suppresses the tracker.close() spam.
3. Fallback precedence is inverted. addFallbackTypeConverter does fallbackConverters.add(0, ...), i.e. last-added wins, which is what the "install the high ranking fallback converter at last" sort depends on. Running the replay after the whole loader loop puts every programmatic fallback in front of the high-ranking loader fallback, reversing the live ordering on any rebuild.

The rest are lower-stakes but worth a look, particularly the now-fully-synchronized getDelegate() on the conversion hot path, and the fact that the keyed map only prunes the addTypeConverter/addConverter pair — addTypeConverters has no removal API and takes a fresh bean instance on every Blueprint refresh, so the identity key appends rather than replaces on the path the description names as motivating.

Two process notes:

• Commits 2c0676e26 and 504ef8f0c carry Co-Authored-By: trailers. The operator's global agent-guidelines file states, under Git: "Do NOT include Co-Authored-By lines in git commit messages". main is squash-merged, so whichever message is used for the squash can carry the trailer into permanent history — please strip them before merge.
• Worth re-checking whether deferring the getDelegate() concurrency items to #743 still makes sense. The deferral was reasoned about against the pre-PR shape, where delegate was a lock-free volatile read. This PR widens getDelegate() to a full monitor, which makes the removedService swallowed-invalidation window larger, not smaller — so the follow-up is arguably now caused in part by this changeset rather than merely inherited by it. Either closing it here or noting explicitly in the description that the window was widened knowingly would help.

Filed as comments, not a change request — the merge/blocking call is yours.

[jbonofre]

The replay is outside any try/catch, so one throwing registration permanently bricks the registry — and with TypeConverterExists.Fail it is guaranteed to throw.

I disassembled CoreTypeConverterRegistry from camel-base-4.18.1.jar:

• addConverter(TypeConvertible, TypeConverter) is a bare converters.put(...) — no duplicate check.
• addTypeConverter(Class, Class, TypeConverter) routes through addOrReplaceTypeConverter() -> onTypeConverterExists(), which returns true for Override, false for Ignore, and otherwise throws TypeConverterExistsException.

So:

1. Context is configured typeConverterExists=Fail.
2. A Blueprint bean calls addTypeConverter(Foo, Bar, tc) while only core converters are loaded — no conflict, applied and recorded.
3. Bundle X's TypeConverterLoader later registers Bar -> Foo via addConverter() (plain put, no check).
4. Any loader unregisters; the delegate is discarded.
5. Next getDelegate(): loaders load first, then this replay calls addTypeConverter(Foo, Bar, tc) -> existing converter found -> TypeConverterExistsException escapes createRegistry() and getDelegate().

delegate stays null, so every convertTo / tryConvertTo / lookup in the container throws forever, and each retry re-throws because the offending entry is still in programmaticRegistrations. The loader loop just above is wrapped in try/catch (Throwable); the replay is not.

Even on the default path this is lossy rather than fatal: the constructor sets typeConverterExists = Ignore and typeConverterExistsLoggingLevel = LoggingLevel.DEBUG, so the same collision silently drops the programmatic converter and logs only at DEBUG. programmaticConverterSurvivesARegistryRebuild does not catch it because the Marker interface is chosen specifically so the pair cannot collide with a core converter.

[jbonofre]

!isStopped() also suppresses this warning in every pre-STARTED state, so the diagnostic #734 asked for is missing exactly in the lazy-init window this class documents.

From BaseService (camel-api 4.18):

public boolean isStopped() {
    return status < STARTING || status >= STOPPED;
}

with NEW = 0, BUILT = 1, INITIALIZING = 2, INITIALIZED = 3, STARTING = 4 ... STOPPED = 9 ... FAILED = 12.

So isStopped() is true for NEW, BUILT, INITIALIZING, INITIALIZED and FAILED — not just after a stop. getDelegate()'s own comment below says it "may be called during doInit() ... which happens before doStart()". An OsgiTypeConverter that is already serving conversions but has not reached STARTED will therefore discard its registry on a loader unregistration with no log line at all, which is the invisibility this PR exists to remove.

isStopping() alone already achieves the stated goal (no line-per-loader on tracker.close(), since stop() sets status = STOPPING before calling doStop()). The && !isStopped() half is what breaks it; if a post-stop guard is also wanted, status != STOPPED is the narrow form.

[jbonofre]

Replaying after the loader loop inverts fallback converter precedence, because addFallbackTypeConverter prepends.

CoreTypeConverterRegistry.addFallbackTypeConverter (verified in bytecode) is:

fallbackConverters.add(0, new FallbackTypeConverter(typeConverter, canPromote));

i.e. last added == index 0 == tried first. That is what createRegistry()'s "Just make sure we install the high ranking fallback converter at last" comment relies on: the highest-ranking ServiceReference sorts last, so its fallback lands at index 0.

replayProgrammaticRegistrations(answer) then runs after the entire loop, so any programmatic fallback is prepended in front of the high-ranking loader fallback that the sort deliberately put there.

In the live registry the opposite ordering held: a fallback registered at time T sat behind every loader that arrived after T (addingService -> loader.load(delegate) -> prepend). So a single loader unregistration silently flips which fallback wins for every unmatched conversion in the context. The field Javadoc's claim that the rebuilt registry is brought "back to the same state" does not hold for fallbacks.

[jbonofre]

The keyed map only prunes the addTypeConverter / addConverter pair. addTypeConverters, addBulkTypeConverters and addFallbackTypeConverter are keyed by contributed-instance identity and have no removal API, so the map still grows without bound and still pins bundle classloaders.

removeTypeConverter is the only pruning path and it only deletes a TypeConvertible key. TypeConverterRegistry exposes no removeTypeConverters / removeFallbackTypeConverter / removeBulkTypeConverters, so entries recorded here (and at the addBulkTypeConverters / addFallbackTypeConverter calls below) live until doStop().

That breaks the stated rationale for keying on the affected path. "Re-registering replaces rather than appends, which is what a Blueprint container refresh does" is true for addTypeConverter, but a container refresh instantiates a new bean, so addTypeConverters(newBeanInstance) hashes to a fresh identity key and appends. 50 refreshes of a bundle contributing a TypeConverters bean leave 50 entries, each strongly holding the bean and through it the classloader of a bundle that may already be uninstalled — the leak shape the review flagged, on the path the PR description names as its motivating case. addRemovePairsDoNotAccumulate only exercises addTypeConverter / removeTypeConverter.

Secondary point: all three share one untyped key space, and BulkTypeConverters extends Ordered, TypeConverter, so the same instance passed to both addBulkTypeConverters and addFallbackTypeConverter collides and one registration is dropped from the replay. (CoreTypeConverterRegistry.addBulkTypeConverters is also a bare return in 4.18, so recording that one is dead weight either way.)

[jbonofre]

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.

[jbonofre]

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.

[jbonofre]

delegate is read here without the monitor, so a loader arriving during a rebuild can end up recorded in trackedLoaders but present in no registry, with nothing left to trigger another rebuild.

1. T1 in synchronized getDelegate(): delegate == null, enters createRegistry(), executes new ArrayList<>(trackedLoaders.keySet()) — empty.
2. T2 (framework dispatch, addingService for sr_A): trackedLoaders.put(sr_A, loader) succeeds, then this line reads delegate as null because T1 has not assigned yet, so the if (current != null) branch is skipped and load() is never called.
3. T1 completes and publishes a registry that does not contain sr_A's converters.

sr_A is in trackedLoaders, so no further service event fires and the next rebuild only happens on an unrelated unregistration. Until then every conversion sr_A provided fails with NoTypeConversionAvailableException.

Recording the loader and deciding whether to load it are two steps raced against a build that is atomic with neither. Note the pre-PR code had the same hole via tracker.getServiceReferences() (the tracker's tracked map is only populated after addingService returns), so this is not a regression — but it is in the method being rewritten, and the trackedLoaders map is now the place where it could actually be closed.

[jbonofre]

I think it's good enough to be merged. We can always improve later.