You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2022/04/12 08:31:23 UTC
[GitHub] [celix] xuzhenbao opened a new pull request, #414: Fix rsalist race
xuzhenbao opened a new pull request, #414:
URL: https://github.com/apache/celix/pull/414
If during topologyManager_addImportedService or topologyManager_addExportedService, topologyManager_rsaRemoved happened. It may cause an invalid RSA to be added to hashmap of imoorts or exports.
If during topologyManager_addImportedService or topologyManager_addExportedService, topologyManager_rsaAdded happened.The same service may be imported or exported twice.
The following is the stack information when the problem occurs.
~~~"bash"
==18087==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0000262a0 at pc 0x7fffebcf2333 bp 0x7fffe3edfb30 sp 0x7fffe3edfb20
READ of size 8 at 0x60e0000262a0 thread T12
#0 0x7fffebcf2332 in topologyManager_removeImportedService /home/xuzhenbao/code/celix/bundles/remote_services/topology_manager/src/topology_manager.c:532
#1 0x7fffe71dff40 in discovery_informEndpointListeners /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/discovery.c:177
#2 0x7fffe71e025f in discovery_removeDiscoveredEndpoint /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/discovery.c:228
#3 0x7fffe71e4266 in endpointDiscoveryPoller_poll /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:265
#4 0x7fffe71e45b3 in endpointDiscoveryPoller_performPeriodicPoll /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:312
#5 0x7ffff5a1c6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#6 0x7ffff574561e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x12161e)
0x60e0000262a0 is located 128 bytes inside of 160-byte region [0x60e000026220,0x60e0000262c0)
freed by thread T11 here:
#0 0x7ffff6ef67a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8)
#1 0x7fffeba95257 in celix_bundleActivator_destroy /home/xuzhenbao/code/celix/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_activator.c:72
#2 0x7ffff6bb7119 in celix_framework_stopBundleEntry /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2391
#3 0x7ffff6bec90e in celix_framework_stopStartBundleThread /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:36
#4 0x7ffff5a1c6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
previously allocated by thread T0 here:
#0 0x7ffff6ef6d28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
#1 0x7fffeba951ac in celix_bundleActivator_create /home/xuzhenbao/code/celix/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_activator.c:72
#2 0x7ffff6bb867d in celix_framework_startBundleEntry /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2563
#3 0x7ffff6bed20d in celix_framework_startBundleOnANonCelixEventThread /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:113
#4 0x7ffff6bb7a73 in celix_framework_startBundleInternal /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2455
#5 0x7ffff6bb7acd in celix_framework_startBundle /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2465
#6 0x7ffff6ba9f83 in framework_autoStartConfiguredBundlesForList /home/xuzhenbao/code/celix/libs/framework/src/framework.c:588
#7 0x7ffff6ba99b5 in framework_autoStartConfiguredBundles /home/xuzhenbao/code/celix/libs/framework/src/framework.c:543
#8 0x7ffff6ba9389 in framework_start /home/xuzhenbao/code/celix/libs/framework/src/framework.c:517
#9 0x7ffff6be379f in celix_frameworkFactory_createFramework /home/xuzhenbao/code/celix/libs/framework/src/celix_framework_factory.c:34
#10 0x7ffff6be2ca5 in celixLauncher_launchWithProperties /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:158
#11 0x7ffff6be2c7d in celixLauncher_launchWithConfigAndProps /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:149
#12 0x7ffff6be2a13 in celixLauncher_launchAndWaitForShutdown /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:105
#13 0x555555554ad8 in main /home/xuzhenbao/code/celix/build/celix/gen/containers/remote-services-dfi-client/main.c:19
#14 0x7ffff5645c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
Thread T12 created by T0 here:
#0 0x7ffff6e4fd2f in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x37d2f)
#1 0x7ffff670548e in celixThread_create /home/xuzhenbao/code/celix/libs/utils/src/celix_threads.c:38
#2 0x7fffe71e3736 in endpointDiscoveryPoller_create /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:115
#3 0x7fffe71dedef in discovery_start /home/xuzhenbao/code/celix/bundles/remote_services/discovery_configured/src/discovery_impl.c:68
#4 0x7fffe71e096e in bundleActivator_start /home/xuzhenbao/code/celix/bundles/remote_services/discovery_common/src/discovery_activator.c:125
#5 0x7ffff6bb8772 in celix_framework_startBundleEntry /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2571
#6 0x7ffff6bed20d in celix_framework_startBundleOnANonCelixEventThread /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:113
#7 0x7ffff6bb7a73 in celix_framework_startBundleInternal /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2455
#8 0x7ffff6bb7acd in celix_framework_startBundle /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2465
#9 0x7ffff6ba9f83 in framework_autoStartConfiguredBundlesForList /home/xuzhenbao/code/celix/libs/framework/src/framework.c:588
#10 0x7ffff6ba99b5 in framework_autoStartConfiguredBundles /home/xuzhenbao/code/celix/libs/framework/src/framework.c:543
#11 0x7ffff6ba9389 in framework_start /home/xuzhenbao/code/celix/libs/framework/src/framework.c:517
#12 0x7ffff6be379f in celix_frameworkFactory_createFramework /home/xuzhenbao/code/celix/libs/framework/src/celix_framework_factory.c:34
#13 0x7ffff6be2ca5 in celixLauncher_launchWithProperties /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:158
#14 0x7ffff6be2c7d in celixLauncher_launchWithConfigAndProps /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:149
#15 0x7ffff6be2a13 in celixLauncher_launchAndWaitForShutdown /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:105
#16 0x555555554ad8 in main /home/xuzhenbao/code/celix/build/celix/gen/containers/remote-services-dfi-client/main.c:19
#17 0x7ffff5645c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
Thread T11 created by T9 here:
#0 0x7ffff6e4fd2f in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x37d2f)
#1 0x7ffff670548e in celixThread_create /home/xuzhenbao/code/celix/libs/utils/src/celix_threads.c:38
#2 0x7ffff6bed0b3 in celix_framework_createAndStartBundleLifecycleHandler /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:98
#3 0x7ffff6bed2a5 in celix_framework_stopBundleOnANonCelixEventThread /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:120
#4 0x7ffff6bb6a83 in celix_framework_stopBundleInternal /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2317
#5 0x7ffff6bb6b84 in celix_framework_stopBundleAsync /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2335
#6 0x7fffe805c721 in stopCommand_execute /home/xuzhenbao/code/celix/bundles/shell/shell/src/stop_command.c:50
#7 0x7fffe80589a8 in shell_executeCommand /home/xuzhenbao/code/celix/bundles/shell/shell/src/c_shell.c:276
#8 0x7fffe7e49aea in shellTui_parseInputPlain /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/shell_tui.c:283
#9 0x7fffe7e498b1 in shellTui_parseInput /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/shell_tui.c:265
#10 0x7fffe7e49562 in shellTui_runnable /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/shell_tui.c:233
#11 0x7ffff5a1c6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
Thread T9 created by T0 here:
#0 0x7ffff6e4fd2f in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x37d2f)
#1 0x7ffff670548e in celixThread_create /home/xuzhenbao/code/celix/libs/utils/src/celix_threads.c:38
#2 0x7fffe7e48920 in shellTui_start /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/shell_tui.c:127
#3 0x7fffe7e48131 in celix_shellTuiActivator_start /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/activator.c:83
#4 0x7fffe7e4839a in celix_bundleActivator_start /home/xuzhenbao/code/celix/bundles/shell/shell_tui/src/activator.c:101
#5 0x7ffff6bb8772 in celix_framework_startBundleEntry /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2571
#6 0x7ffff6bed20d in celix_framework_startBundleOnANonCelixEventThread /home/xuzhenbao/code/celix/libs/framework/src/framework_bundle_lifecycle_handler.c:113
#7 0x7ffff6bb7a73 in celix_framework_startBundleInternal /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2455
#8 0x7ffff6bb7acd in celix_framework_startBundle /home/xuzhenbao/code/celix/libs/framework/src/framework.c:2465
#9 0x7ffff6ba9f83 in framework_autoStartConfiguredBundlesForList /home/xuzhenbao/code/celix/libs/framework/src/framework.c:588
#10 0x7ffff6ba99b5 in framework_autoStartConfiguredBundles /home/xuzhenbao/code/celix/libs/framework/src/framework.c:543
#11 0x7ffff6ba9389 in framework_start /home/xuzhenbao/code/celix/libs/framework/src/framework.c:517
#12 0x7ffff6be379f in celix_frameworkFactory_createFramework /home/xuzhenbao/code/celix/libs/framework/src/celix_framework_factory.c:34
#13 0x7ffff6be2ca5 in celixLauncher_launchWithProperties /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:158
#14 0x7ffff6be2c7d in celixLauncher_launchWithConfigAndProps /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:149
#15 0x7ffff6be2a13 in celixLauncher_launchAndWaitForShutdown /home/xuzhenbao/code/celix/libs/framework/src/celix_launcher.c:105
#16 0x555555554ad8 in main /home/xuzhenbao/code/celix/build/celix/gen/containers/remote-services-dfi-client/main.c:19
#17 0x7ffff5645c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
SUMMARY: AddressSanitizer: heap-use-after-free celix/bundles/remote_services/topology_manager/src/topology_manager.c:532 in topologyManager_removeImportedService
~~~
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r856888330
##########
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:
As for suitable logging levels, shall we adopt the interpretation of https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels ?
If we change level here, `topologyManager_removeImportedService_nolock`/`topologyManager_addExportedService_nolock`/`topologyManager_removeExportedService_nolock` should also be changed to keep everything consistent.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on PR #414:
URL: https://github.com/apache/celix/pull/414#issuecomment-1097494112
> I prefer that we require the RSAs to start service tracker / register service async during export / import and simplify the remote service topology manager.
Can Component A still register services both sync and async? Existing users should not be affected by this change.
It seems that https://github.com/apache/celix/pull/414/commits/0913af41282e029fc19b03b930267ba5829c4608 does not have ABBA problem, locks are always acquired in order exportedServicesLock->importedServicesLock->rsaListLock.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r848212101
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -477,33 +479,32 @@ celix_status_t topologyManager_addImportedService(void *handle, endpoint_descrip
celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO, "TOPOLOGY_MANAGER: Add imported service (%s; %s).", endpoint->service, endpoint->id);
+ celixThreadMutex_lock(&manager->rsaListLock);
Review Comment:
Considering services frequently go up and down, it does not seem a good idea to unconditionally lock rtsListLock.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on PR #414:
URL: https://github.com/apache/celix/pull/414#issuecomment-1096658568
These race conditions are not very easy to reproduce. Xu utilized GDB to suspend certain thread at specific point to enlarge normally very small race window.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r849022810
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -288,6 +289,8 @@ celix_status_t topologyManager_rsaRemoved(void * handle, service_reference_pt re
remote_service_admin_service_t *rsa = (remote_service_admin_service_t *) service;
celixThreadMutex_lock(&manager->rsaListLock);
+ arrayList_removeElement(manager->rsaList, rsa);
+ celixThreadMutex_unlock(&manager->rsaListLock);
Review Comment:
`topologyManager_rsaRemoved` should be dual to `topologyManager_rsaAdded`: things should be done in strictly reverse order.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] xuzhenbao commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
xuzhenbao commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r849589747
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -288,6 +289,8 @@ celix_status_t topologyManager_rsaRemoved(void * handle, service_reference_pt re
remote_service_admin_service_t *rsa = (remote_service_admin_service_t *) service;
celixThreadMutex_lock(&manager->rsaListLock);
+ arrayList_removeElement(manager->rsaList, rsa);
+ celixThreadMutex_unlock(&manager->rsaListLock);
Review Comment:
Thank you for your valuable suggestions.
@pnoltes , If I understand your opinion, we can delete rsaListLock, exportedServicesLock and importedServicesLock , but add a single mutex. Then we can use the new mutex protect rsaList,exportedServices, importedServices and relevant operation.
If I am right, I will also fix issue #415 in this PR.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
pnoltes commented on PR #414:
URL: https://github.com/apache/celix/pull/414#issuecomment-1097040371
If I remember correctly the remote service topology manager has 3 locks and even 2 as recursive locks, because registering service during import can loop back to the topology manager.
For example:
- Component A has a required dependency to Service1 and
- Component A provides a - marked for remote - Service2 when active
- Topology manager import a remote Service1 -> during importService, this activates Component A -> during importService, topology manager tries to export Service2.
But a while back the dfi export_registration has been updated to track service trackers async and (more importantly) the dfi import_registration has been updated to register service async:
https://github.com/apache/celix/blob/0913af41282e029fc19b03b930267ba5829c4608/bundles/remote_services/remote_service_admin_dfi/src/import_registration_dfi.c#L155
This means that a import/export service can be safely completed before the service tracker / service registration is picked up by the Celix event thread.
I prefer that we require the RSAs to start service tracker / register service async during export / import and simplify the remote service topology manager.
If you agree, I think we should refactor the topology manager to use a single lock or mutex and thus avoid the whole ABBA situation.
WDYT?
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r856889636
##########
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:
If we extend managed tracker (created by `celix_bundleContext_trackServices`) API to support CELIX_SERVICE_USE_DIRECT, as already discussed here: https://github.com/apache/celix/pull/399#discussion_r816607673, then a lot of external synchronizations, such as `discovery->discoveredServicesMutex`, can be eliminated.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r856888923
##########
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:
@pnoltes This is an old comment. Xu is thinking about improvement of this.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r849764553
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -288,6 +289,8 @@ celix_status_t topologyManager_rsaRemoved(void * handle, service_reference_pt re
remote_service_admin_service_t *rsa = (remote_service_admin_service_t *) service;
celixThreadMutex_lock(&manager->rsaListLock);
+ arrayList_removeElement(manager->rsaList, rsa);
+ celixThreadMutex_unlock(&manager->rsaListLock);
Review Comment:
Yes correct. a single mutex in the topology manager should be enough.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r848206922
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -209,7 +209,6 @@ celix_status_t topologyManager_rsaAdded(void * handle, service_reference_pt unus
celixThreadMutex_lock(&manager->rsaListLock);
arrayList_add(manager->rsaList, rsa);
- celixThreadMutex_unlock(&manager->rsaListLock);
// add already imported services to new rsa
celixThreadMutex_lock(&manager->importedServicesLock);
Review Comment:
Then we have to make sure ABBA deak-lock does not happen: `rsaListLock` should always be acquired before `importedServicesLock`.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
pnoltes commented on PR #414:
URL: https://github.com/apache/celix/pull/414#issuecomment-1098345718
> > I prefer that we require the RSAs to start service tracker / register service async during export / import and simplify the remote service topology manager.
>
> Can Component A still register services both sync and async? If all existing users are not affected, I'd prefer this simpler approach.
A small correction. The component service registration is also updated.
In the current Celix the previous example is not correct anymore, because component A will also register its provided service async:
https://github.com/apache/celix/blob/master/libs/framework/src/dm_component_impl.c#L858
@PengZheng currently components will always provide services async. If it is desirable it should be possible to make this configureable.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r849022810
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -288,6 +289,8 @@ celix_status_t topologyManager_rsaRemoved(void * handle, service_reference_pt re
remote_service_admin_service_t *rsa = (remote_service_admin_service_t *) service;
celixThreadMutex_lock(&manager->rsaListLock);
+ arrayList_removeElement(manager->rsaList, rsa);
+ celixThreadMutex_unlock(&manager->rsaListLock);
Review Comment:
`topologyManager_rsaRemoved` should be dual to `topologyManager_rsaAdded`: things should be done in **strictly reverse order**.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng merged pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng merged PR #414:
URL: https://github.com/apache/celix/pull/414
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] codecov-commenter commented on pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #414:
URL: https://github.com/apache/celix/pull/414#issuecomment-1096374564
# [Codecov](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#414](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e913672) into [master](https://codecov.io/gh/apache/celix/commit/9cf8bc39c1a8916fbdda54eac59b24065f0fe3b2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9cf8bc3) will **decrease** coverage by `1.32%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #414 +/- ##
==========================================
- Coverage 73.12% 71.80% -1.33%
==========================================
Files 205 180 -25
Lines 31274 29287 -1987
==========================================
- Hits 22870 21029 -1841
+ Misses 8404 8258 -146
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...les/pubsub/pubsub\_admin\_zmq/src/pubsub\_zmq\_admin.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS9zcmMvcHVic3ViX3ptcV9hZG1pbi5j) | `52.96% <0.00%> (-13.93%)` | :arrow_down: |
| [libs/framework/include/celix/dm/Component\_Impl.h](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9Db21wb25lbnRfSW1wbC5o) | `82.81% <0.00%> (-10.53%)` | :arrow_down: |
| [...ibs/pushstreams/api/celix/impl/PushEventConsumer.h](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wdXNoc3RyZWFtcy9hcGkvY2VsaXgvaW1wbC9QdXNoRXZlbnRDb25zdW1lci5o) | `90.00% <0.00%> (-10.00%)` | :arrow_down: |
| [...ibs/framework/include/celix/dm/ServiceDependency.h](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9TZXJ2aWNlRGVwZW5kZW5jeS5o) | `92.85% <0.00%> (-7.15%)` | :arrow_down: |
| [...b/pubsub\_admin\_zmq/src/pubsub\_zmq\_topic\_receiver.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS9zcmMvcHVic3ViX3ptcV90b3BpY19yZWNlaXZlci5j) | `76.50% <0.00%> (-4.82%)` | :arrow_down: |
| [...ramework/include/celix/dm/ServiceDependency\_Impl.h](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9kbS9TZXJ2aWNlRGVwZW5kZW5jeV9JbXBsLmg=) | `93.70% <0.00%> (-1.63%)` | :arrow_down: |
| [...sub/pubsub\_admin\_zmq/src/pubsub\_zmq\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS9zcmMvcHVic3ViX3ptcV90b3BpY19zZW5kZXIuYw==) | `85.65% <0.00%> (-1.27%)` | :arrow_down: |
| [bundles/pubsub/pubsub\_utils/src/pubsub\_utils.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX3V0aWxzL3NyYy9wdWJzdWJfdXRpbHMuYw==) | `68.88% <0.00%> (-1.12%)` | :arrow_down: |
| [...e\_service\_admin\_dfi/src/remote\_service\_admin\_dfi.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9yZW1vdGVfc2VydmljZXMvcmVtb3RlX3NlcnZpY2VfYWRtaW5fZGZpL3NyYy9yZW1vdGVfc2VydmljZV9hZG1pbl9kZmkuYw==) | `84.06% <0.00%> (-0.98%)` | :arrow_down: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `93.25% <0.00%> (-0.29%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/apache/celix/pull/414/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9cf8bc3...e913672](https://codecov.io/gh/apache/celix/pull/414?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r848212101
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -477,33 +479,32 @@ celix_status_t topologyManager_addImportedService(void *handle, endpoint_descrip
celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO, "TOPOLOGY_MANAGER: Add imported service (%s; %s).", endpoint->service, endpoint->id);
+ celixThreadMutex_lock(&manager->rsaListLock);
Review Comment:
Considering services frequently go up and down and RSA rarely changes, it does not seem a good idea to unconditionally lock rtsListLock.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #414: Fix rsalist race
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #414:
URL: https://github.com/apache/celix/pull/414#discussion_r848212101
##########
bundles/remote_services/topology_manager/src/topology_manager.c:
##########
@@ -477,33 +479,32 @@ celix_status_t topologyManager_addImportedService(void *handle, endpoint_descrip
celix_logHelper_log(manager->loghelper, CELIX_LOG_LEVEL_INFO, "TOPOLOGY_MANAGER: Add imported service (%s; %s).", endpoint->service, endpoint->id);
+ celixThreadMutex_lock(&manager->rsaListLock);
Review Comment:
It does not seem a good idea to unconditionally lock rtsListLock.
--
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-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org