pnoltes commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r853402486
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -49,19 +49,16 @@
struct topology_manager {
celix_bundle_context_t *context;
- celix_thread_mutex_t rsaListLock;
- celix_thread_mutexattr_t rsaListLockAttr;
array_list_pt rsaList;
- celix_thread_mutex_t listenerListLock;
hash_map_pt listenerList;
- celix_thread_mutex_t exportedServicesLock;
hash_map_pt exportedServices;
- celix_thread_mutex_t importedServicesLock;
- celix_thread_mutexattr_t importedServicesLockAttr;
hash_map_pt importedServices;
+
+ celix_thread_mutex_t lock;
Review Comment:
nitpick: please specify in comments what fields the lock protects
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -446,122 +418,138 @@ celix_status_t topologyManager_importScopeChanged(void
*handle, char *service_na
topology_manager_pt manager = (topology_manager_pt) handle;
bool found = false;
- // add already exported services to new rsa
- if (celixThreadMutex_lock(&manager->importedServicesLock) ==
CELIX_SUCCESS) {
- hash_map_iterator_pt importedServicesIterator =
hashMapIterator_create(manager->importedServices);
- while (!found &&
hashMapIterator_hasNext(importedServicesIterator)) {
- hash_map_entry_pt entry =
hashMapIterator_nextEntry(importedServicesIterator);
- endpoint = hashMapEntry_getKey(entry);
-
- entry = hashMap_getEntry(endpoint->properties, (void *)
OSGI_FRAMEWORK_OBJECTCLASS);
- char* name = (char *) hashMapEntry_getValue(entry);
- // Test if a service with the same name is imported
- if (strcmp(name, service_name) == 0) {
- found = true;
- }
+ // add already imported services to new rsa
+ celixThreadMutex_lock(&manager->lock);
+
+ hash_map_iterator_pt importedServicesIterator =
hashMapIterator_create(manager->importedServices);
+ while (!found && hashMapIterator_hasNext(importedServicesIterator)) {
+ hash_map_entry_pt entry =
hashMapIterator_nextEntry(importedServicesIterator);
+ endpoint = hashMapEntry_getKey(entry);
+
+ entry = hashMap_getEntry(endpoint->properties, (void *)
OSGI_FRAMEWORK_OBJECTCLASS);
+ char* name = (char *) hashMapEntry_getValue(entry);
+ // Test if a service with the same name is imported
+ if (strcmp(name, service_name) == 0) {
+ found = true;
}
- hashMapIterator_destroy(importedServicesIterator);
- celixThreadMutex_unlock(&manager->importedServicesLock);
}
+ hashMapIterator_destroy(importedServicesIterator);
if (found) {
- status = topologyManager_removeImportedService(manager,
endpoint, NULL);
+ status = topologyManager_removeImportedService_nolock(manager,
endpoint, NULL);
if (status != CELIX_SUCCESS) {
celix_logHelper_log(manager->loghelper,
CELIX_LOG_LEVEL_ERROR, "TOPOLOGY_MANAGER: Removal of imported service (%s; %s)
failed.", endpoint->service, endpoint->id);
} else {
- status = topologyManager_addImportedService(manager,
endpoint, NULL);
+ status =
topologyManager_addImportedService_nolock(manager, endpoint, NULL);
}
}
+
+ //should unlock until here ?, avoid endpoint is released during
topologyManager_removeImportedService
+ celixThreadMutex_unlock(&manager->lock);
+
return status;
}
-celix_status_t topologyManager_addImportedService(void *handle,
endpoint_description_t *endpoint, char *matchedFilter) {
+static celix_status_t topologyManager_addImportedService_nolock(void *handle,
endpoint_description_t *endpoint, char *matchedFilter) {
celix_status_t status = CELIX_SUCCESS;
topology_manager_pt manager = handle;
celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO,
"TOPOLOGY_MANAGER: Add imported service (%s; %s).", endpoint->service,
endpoint->id);
Review Comment:
IMO this should be a DEBUG (or maybe TRACE), but no a INFO log
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -119,36 +112,18 @@ celix_status_t
topologyManager_create(celix_bundle_context_t *context, celix_log
celix_status_t topologyManager_destroy(topology_manager_pt manager) {
celix_status_t status = CELIX_SUCCESS;
- celixThreadMutex_lock(&manager->listenerListLock);
- hashMap_destroy(manager->listenerList, false, false);
-
- celixThreadMutex_unlock(&manager->listenerListLock);
- celixThreadMutex_destroy(&manager->listenerListLock);
-
- celixThreadMutex_lock(&manager->rsaListLock);
-
- arrayList_destroy(manager->rsaList);
-
- celixThreadMutex_unlock(&manager->rsaListLock);
- celixThreadMutex_destroy(&manager->rsaListLock);
- celixThreadMutexAttr_destroy(&manager->rsaListLockAttr);
-
- celixThreadMutex_lock(&manager->exportedServicesLock);
-
- hashMap_destroy(manager->exportedServices, false, false);
-
- celixThreadMutex_unlock(&manager->exportedServicesLock);
- celixThreadMutex_destroy(&manager->exportedServicesLock);
+ scope_scopeDestroy(manager->scope);
- celixThreadMutex_lock(&manager->importedServicesLock);
+ celixThreadMutex_lock(&manager->lock);
hashMap_destroy(manager->importedServices, false, false);
+ hashMap_destroy(manager->exportedServices, false, false);
+ hashMap_destroy(manager->listenerList, false, false);
+ arrayList_destroy(manager->rsaList);
Review Comment:
nitpick: please replace with celix_ prefix array list api:
`celix_arrayList_destroy(manager->rsaList);`
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -290,75 +264,70 @@ celix_status_t topologyManager_rsaRemoved(void * handle,
service_reference_pt re
topology_manager_pt manager = (topology_manager_pt) handle;
remote_service_admin_service_t *rsa = (remote_service_admin_service_t
*) service;
- if (celixThreadMutex_lock(&manager->exportedServicesLock) ==
CELIX_SUCCESS) {
- hash_map_iterator_pt iter =
hashMapIterator_create(manager->exportedServices);
+ celixThreadMutex_lock(&manager->lock);
- while (hashMapIterator_hasNext(iter)) {
+ hash_map_iterator_pt exportedSvcIter =
hashMapIterator_create(manager->exportedServices);
- hash_map_entry_pt entry =
hashMapIterator_nextEntry(iter);
- service_reference_pt key = hashMapEntry_getKey(entry);
- hash_map_pt exports = hashMapEntry_getValue(entry);
-
- /*
- * the problem here is that also the rsa has a a list of
- * endpoints which is destroyed when closing the
exportRegistration
- */
- array_list_pt exports_list = hashMap_get(exports, rsa);
-
- if (exports_list != NULL) {
- int exportsIter = 0;
- int exportListSize =
arrayList_size(exports_list);
- for (exportsIter = 0; exports_list != NULL &&
exportsIter < exportListSize; exportsIter++) {
- export_registration_t *export =
arrayList_get(exports_list, exportsIter);
-
topologyManager_notifyListenersEndpointRemoved(manager, rsa, export);
-
rsa->exportRegistration_close(rsa->admin, export);
- }
- }
+ while (hashMapIterator_hasNext(exportedSvcIter)) {
- hashMap_remove(exports, rsa);
- /*if(exports_list!=NULL){
- arrayList_destroy(exports_list);
- }*/
+ hash_map_entry_pt entry =
hashMapIterator_nextEntry(exportedSvcIter);
+ service_reference_pt key = hashMapEntry_getKey(entry);
+ hash_map_pt exports = hashMapEntry_getValue(entry);
- if (hashMap_size(exports) == 0) {
- hashMap_remove(manager->exportedServices, key);
- hashMap_destroy(exports, false, false);
+ /*
+ * the problem here is that also the rsa has a a list of
Review Comment:
Do you mean that the RSA also has a list of the same endpoint pointers or
copies of the same endpoints?
##########
bundles/remote_services/topology_manager/src/activator.c:
##########
@@ -183,6 +183,8 @@ celix_status_t bundleActivator_start(void * userData,
celix_bundle_context_t *co
celix_logHelper_log(activator->celix_logHelper, CELIX_LOG_LEVEL_INFO,
"TOPOLOGY_MANAGER: endpoint listener scope is %s", scope);
celix_properties_t *props = celix_properties_create();
+ // topology manager shoud ingore itself endpoint listener service
Review Comment:
nitpick: should instead of shoud
##########
bundles/remote_services/topology_manager/src/activator.c:
##########
@@ -183,6 +183,8 @@ celix_status_t bundleActivator_start(void * userData,
celix_bundle_context_t *co
celix_logHelper_log(activator->celix_logHelper, CELIX_LOG_LEVEL_INFO,
"TOPOLOGY_MANAGER: endpoint listener scope is %s", scope);
celix_properties_t *props = celix_properties_create();
+ // topology manager shoud ingore itself endpoint listener service
+ celix_properties_set(props, "TOPOLOGY_MANAGER", "true");
Review Comment:
nice and simple solution to help filter out the topology manager 👍
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -86,26 +88,17 @@ celix_status_t
topologyManager_create(celix_bundle_context_t *context, celix_log
(*manager)->context = context;
(*manager)->rsaList = NULL;
- arrayList_create(&(*manager)->rsaList);
-
-
- celixThreadMutexAttr_create(&(*manager)->rsaListLockAttr);
- celixThreadMutexAttr_settype(&(*manager)->rsaListLockAttr,
CELIX_THREAD_MUTEX_RECURSIVE);
- celixThreadMutex_create(&(*manager)->rsaListLock,
&(*manager)->rsaListLockAttr);
-
- celixThreadMutexAttr_create(&(*manager)->importedServicesLockAttr);
- celixThreadMutexAttr_settype(&(*manager)->importedServicesLockAttr,
CELIX_THREAD_MUTEX_RECURSIVE);
- celixThreadMutex_create(&(*manager)->importedServicesLock,
&(*manager)->importedServicesLockAttr);
- (*manager)->closed = false;
-
- celixThreadMutex_create(&(*manager)->exportedServicesLock, NULL);
- celixThreadMutex_create(&(*manager)->listenerListLock, NULL);
+ celixThreadMutex_create(&(*manager)->lock, NULL);
+ arrayList_create(&(*manager)->rsaList);
Review Comment:
nitpick: replace with celix_ prefixed array list api:
```
(*manager)->rsaList = celix_arrayList_create();
```
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -380,61 +349,64 @@ celix_status_t topologyManager_exportScopeChanged(void
*handle, char *filterStr)
}
// add already exported services to new rsa
- if (celixThreadMutex_lock(&manager->exportedServicesLock) ==
CELIX_SUCCESS) {
- hash_map_iterator_pt exportedServicesIterator =
hashMapIterator_create(manager->exportedServices);
- int size = hashMap_size(manager->exportedServices);
- service_reference_pt *srvRefs = (service_reference_pt *)
calloc(size, sizeof(service_reference_pt));
- char **srvIds = (char **) calloc(size, sizeof(char*));
- int nrFound = 0;
-
- found = false;
-
- while (hashMapIterator_hasNext(exportedServicesIterator)) {
- hash_map_entry_pt entry =
hashMapIterator_nextEntry(exportedServicesIterator);
- service_reference_pt reference =
hashMapEntry_getKey(entry);
- reg = NULL;
- serviceReference_getServiceRegistration(reference,
®);
- if (reg != NULL) {
- props = NULL;
- serviceRegistration_getProperties(reg, &props);
- status = filter_match(filter, props, &found);
- if (found) {
- srvRefs[nrFound] = reference;
- serviceReference_getProperty(reference,
OSGI_FRAMEWORK_SERVICE_ID, &serviceId);
- srvIds[nrFound++] = (char*)serviceId;
- }
+ celixThreadMutex_lock(&manager->lock);
+
+ hash_map_iterator_pt exportedServicesIterator =
hashMapIterator_create(manager->exportedServices);
+ int size = hashMap_size(manager->exportedServices);
+ service_reference_pt *srvRefs = (service_reference_pt *) calloc(size,
sizeof(service_reference_pt));
+ char **srvIds = (char **) calloc(size, sizeof(char*));
+ int nrFound = 0;
+
+ found = false;
+
+ while (hashMapIterator_hasNext(exportedServicesIterator)) {
+ hash_map_entry_pt entry =
hashMapIterator_nextEntry(exportedServicesIterator);
+ service_reference_pt reference = hashMapEntry_getKey(entry);
+ reg = NULL;
+ serviceReference_getServiceRegistration(reference, ®);
+ if (reg != NULL) {
+ props = NULL;
+ serviceRegistration_getProperties(reg, &props);
+ status = filter_match(filter, props, &found);
+ if (found) {
+ srvRefs[nrFound] = reference;
+ serviceReference_getProperty(reference,
OSGI_FRAMEWORK_SERVICE_ID, &serviceId);
+ srvIds[nrFound++] = (char*)serviceId;
}
}
+ }
- hashMapIterator_destroy(exportedServicesIterator);
- celixThreadMutex_unlock(&manager->exportedServicesLock);
+ hashMapIterator_destroy(exportedServicesIterator);
- if (nrFound > 0) {
- for (int i = 0; i < nrFound; i++) {
- // Question: can srvRefs become invalid
meanwhile??
- const char* export = NULL;
- serviceReference_getProperty(srvRefs[i], (char
*) OSGI_RSA_SERVICE_EXPORTED_INTERFACES, &export);
+ if (nrFound > 0) {
+ for (int i = 0; i < nrFound; i++) {
+ // Question: can srvRefs become invalid meanwhile??
Review Comment:
Well, to be honest I am not sure. Service references are not protected and
that is why there are not directly used anymore in the later Celix bundles /
Celix api (celix_ prefixed api). That being, said the currently Celix also
updates all service register/unregister events through the Celix event thread
and I think this is called in the Celix event thread, which means the service
reference cannot become invalid.
Ideally (but I think out of scope for this pull request) the services
(endpoint listeners, RSAs, services marked for export, etc) are tracked using
the celix_bundleContext_trackServices calls instead of the current deprecated
usage of directly working on a service tracker with service references.
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -212,12 +187,12 @@ celix_status_t topologyManager_rsaAdded(void * handle,
service_reference_pt unus
remote_service_admin_service_t *rsa = (remote_service_admin_service_t
*) service;
celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO,
"TOPOLOGY_MANAGER: Added RSA");
- celixThreadMutex_lock(&manager->rsaListLock);
- arrayList_add(manager->rsaList, rsa);
- celixThreadMutex_unlock(&manager->rsaListLock);
- // add already imported services to new rsa
- celixThreadMutex_lock(&manager->importedServicesLock);
+ celixThreadMutex_lock(&manager->lock);
+
+ arrayList_add(manager->rsaList, rsa);
Review Comment:
nitpick: please replace with celix_ prefix array list api:
`celix_arrayList_add(manager->rsaList, rsa);`
etc for the rest of the arrayList without a celix_ prefix usage.
--
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: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
