jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3851027145
##########
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:
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.
--
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]
