oscerd opened a new pull request, #739:
URL: https://github.com/apache/camel-karaf/pull/739
Fixes #734
## 1. `getDelegate()` could build the registry twice
```java
if (delegate == null) {
ensureTrackerOpen();
delegate = createRegistry();
}
return delegate;
```
`delegate` is `volatile`, which makes the *read* safe but does not make the
check and the assignment atomic. Two threads arriving together on first
access
each ran `createRegistry()`; one result was returned and the other silently
discarded, along with anything registered on it in the interim.
Replaced with double-checked locking against the volatile field, so the
conversion hot path stays lock-free and first access happens exactly once.
`removedService` and `addingService` now take the same lock, so the delegate
cannot be stopped and nulled while another thread is in the middle of handing
it out.
## 2. Discarding the delegate was silent
Any single `TypeConverterLoader` going away discards the whole delegate.
`createRegistry()` replays the core converters plus the loaders the tracker
currently holds — it *cannot* replay converters added via `addTypeConverter`
or
by a Blueprint bean implementing `TypeConverters`. Those vanish from a
running
context with nothing logged above `trace`.
`removedService` now logs at WARN naming the bundle whose loader went away
and
saying what the rebuild cannot restore. This does not fix the loss, but it
stops
it being invisible — which was the actual complaint.
## What is deliberately *not* changed
The asymmetry with `addingService` is intentional and I have left it alone.
`d54f9a806` (#625, PR #684) specifically *reverted* invalidate-on-add back to
loading into the existing delegate, precisely to preserve
programmatically-added
converters. Making the two paths symmetric would undo that fix.
Genuinely replaying programmatic registrations on rebuild means recording
them
as they are added — a larger design change than this issue warrants. Happy to
open a separate issue if it is worth doing.
## Tests
`concurrentFirstAccessShouldBuildTheRegistryOnce` releases 16 threads onto
`getDelegate()` from a single latch and asserts both that every caller sees
the
same instance and that `createRegistry()` ran exactly once (counted via a
subclass override).
I checked it actually catches the bug rather than just passing: against the
previous `getDelegate()` it fails **3 out of 3 runs** —
```
AssertionFailedError: every caller must see the same registry instance
==> expected: <...OsgiDefaultTypeConverter@44cffc25>
but was: <...OsgiDefaultTypeConverter@2a369e14>
```
— and passes 3 out of 3 with the fix, so it pins the behaviour rather than
being a flaky probe.
```
Tests run: 13, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```
(all 4 pre-existing `OsgiTypeConverterTest` cases, including
`addingServiceShouldLoadIntoExistingDelegate` which guards the #625
behaviour,
still pass.)
---
_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]
