jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3850720591
##########
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:
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.
##########
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:
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.
##########
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:
Same here, `synchronized` should not be used on `removedService()`, else we
can introduce a deadlock.
--
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]
