jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3860502080
##########
core/camel-core-osgi/src/main/java/org/apache/camel/karaf/core/OsgiTypeConverter.java:
##########
@@ -112,6 +150,16 @@ public void
removedService(ServiceReference<TypeConverterLoader> serviceReferenc
this.delegate = null;
Review Comment:
**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.
--
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]
