This is an automated email from the ASF dual-hosted git repository. pengzheng pushed a commit to branch feature/556-osgi-uninstall in repository https://gitbox.apache.org/repos/asf/celix.git
commit ea6a979c451b19d2b4ac8d123c7dedf79f725847 Author: PengZheng <[email protected]> AuthorDate: Fri Jun 2 14:31:18 2023 +0800 Add useActiveBundles. It fixes thread-safety issue in dependency manager (see #561), a null string logging issue in pubsub's topology manager, and a potential crash in query_command.c. --- .../src/pubsub_topology_manager.c | 2 +- bundles/shell/shell/src/query_command.c | 3 +- libs/framework/include/celix_bundle.h | 2 ++ libs/framework/include/celix_framework.h | 41 +++++++++++++++++++--- libs/framework/src/dm_dependency_manager_impl.c | 27 ++++++-------- libs/framework/src/framework.c | 22 +++++++++--- 6 files changed, 69 insertions(+), 28 deletions(-) diff --git a/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c b/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c index a204668b..c73cc587 100644 --- a/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c +++ b/bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c @@ -1344,7 +1344,7 @@ static celix_status_t pubsub_topologyManager_metrics(pubsub_topology_manager_t * if (sm->msgMetrics[j].nrOfMessagesSend == 0 && sm->msgMetrics[j].nrOfMessagesSendFailed == 0 && sm->msgMetrics[j].nrOfSerializationErrors == 0) { continue; } - const char *bndName = NULL; + const char *bndName = "inactive"; celix_bundleContext_useBundle(manager->context, sm->msgMetrics->bndId, &bndName, fetchBundleName); fprintf(os, " |- Message '%s' from bundle '%s' (%li):\n", sm->msgMetrics[j].typeFqn, bndName, sm->msgMetrics->bndId); fprintf(os, " |- msg type = 0x%X\n", sm->msgMetrics[j].typeId); diff --git a/bundles/shell/shell/src/query_command.c b/bundles/shell/shell/src/query_command.c index ff7f7a8a..490513b9 100644 --- a/bundles/shell/shell/src/query_command.c +++ b/bundles/shell/shell/src/query_command.c @@ -20,6 +20,7 @@ #include <string.h> #include <stdlib.h> +#include "celix_framework.h" #include "celix_shell_constants.h" #include "celix_bundle_context.h" #include "std_commands.h" @@ -129,7 +130,7 @@ static void queryCommand_callback(void *handle, const celix_bundle_t *bnd) { static void queryCommand_listServicesForBundle(celix_bundle_context_t *ctx, long bndId, struct bundle_callback_data *data, const struct query_options *opts, FILE *sout, FILE *serr) { - bool called = celix_bundleContext_useBundle(ctx, bndId, data, queryCommand_callback); + bool called = celix_framework_useBundle(celix_bundleContext_getFramework(ctx), true, bndId, data, queryCommand_callback); if (!called) { fprintf(serr, "Bundle %li not installed!", bndId); } diff --git a/libs/framework/include/celix_bundle.h b/libs/framework/include/celix_bundle.h index 63121eda..fb348709 100644 --- a/libs/framework/include/celix_bundle.h +++ b/libs/framework/include/celix_bundle.h @@ -177,6 +177,8 @@ typedef struct celix_bundle_service_tracker_list_entry { /** * Returns a array list of service tracker info entries for this bundle. * + * @warning It requires a valid bundle context. Calling it for an inactive bundle will lead to crash. + * * @param ctx The bundle context * @param bndId The bundle id for which the services should be listed * @return A celix array list with celix_bundle_service_tracker_list_entry_t*. Caller is owner of the celix array. diff --git a/libs/framework/include/celix_framework.h b/libs/framework/include/celix_framework.h index 2bfbd114..5f3d338a 100644 --- a/libs/framework/include/celix_framework.h +++ b/libs/framework/include/celix_framework.h @@ -74,22 +74,51 @@ CELIX_FRAMEWORK_EXPORT celix_bundle_context_t* celix_framework_getFrameworkConte CELIX_FRAMEWORK_EXPORT celix_bundle_t* celix_framework_getFrameworkBundle(const celix_framework_t *fw); /** - * @brief * @brief Use the currently installed bundles. + * @brief Use the currently installed bundles. * The provided callback will be called for all the currently installed bundles. * - * @param ctx The bundle context. + * @warning It is dangerous to use the provided bundle's context from the callback, since it may be invalid for an inactive bundle. + * + * @param fw The framework. * @param includeFrameworkBundle If true the callback will also be triggered for the framework bundle. * @param callbackHandle The data pointer, which will be used in the callbacks * @param use The callback which will be called for the currently installed bundles. * The bundle pointers are only guaranteed to be valid during the callback. * @return The number of times the use callback is called. */ -CELIX_FRAMEWORK_EXPORT size_t celix_framework_useBundles(celix_framework_t *fw, bool includeFrameworkBundle, void *callbackHandle, void(*use)(void *handle, const celix_bundle_t *bnd)); +CELIX_FRAMEWORK_EXPORT size_t celix_framework_useBundles(celix_framework_t* fw, + bool includeFrameworkBundle, + void* callbackHandle, + void (*use)(void* handle, const celix_bundle_t* bnd)); + +/** + * @brief Use the currently active bundles. + * The provided callback will be called for all the currently active bundles. + * + * @warning Calling synchronous bundle-state changing functions (e.g. celix_bundleContext_stopBundle) from the callback + * will lead to deadlocks. + * + * @param fw The framework. + * @param includeFrameworkBundle If true the callback will also be triggered for the framework bundle. + * @param callbackHandle The data pointer, which will be used in the callbacks + * @param use The callback which will be called for the currently active bundles. + * The bundle state is guaranteed to be active during the callback. + * @return The number of times the use callback is called. + */ +CELIX_FRAMEWORK_EXPORT size_t celix_framework_useActiveBundles(celix_framework_t* fw, + bool includeFrameworkBundle, + void* callbackHandle, + void (*use)(void* handle, const celix_bundle_t* bnd)); /** * @brief Use the bundle with the provided bundle id * The provided callback will be called if the bundle is found. * + * @warning Calling synchronous bundle-state changing functions (e.g. celix_bundleContext_stopBundle) with onlyActive=true + * from the callback will lead to deadlocks. Using a bundle's context, e.g. calling celix_bundle_listServiceTrackers, + * with onlyActive=false from the callback is generally dangerous. However, in some cases, the target bundle's context is guaranteed to be valid, + * e.g. the bundle is providing a service protected by a service tracker. + * * @param fw The framework. * @param onlyActive If true only starting and active bundles will trigger the callback. * @param bundleId The bundle id. @@ -98,7 +127,11 @@ CELIX_FRAMEWORK_EXPORT size_t celix_framework_useBundles(celix_framework_t *fw, * The bundle pointers are only guaranteed to be valid during the callback. * @return Returns true if the bundle is found and the callback is called. */ -CELIX_FRAMEWORK_EXPORT bool celix_framework_useBundle(celix_framework_t *fw, bool onlyActive, long bndId, void *callbackHandle, void(*use)(void *handle, const celix_bundle_t *bnd)); +CELIX_FRAMEWORK_EXPORT bool celix_framework_useBundle(celix_framework_t* fw, + bool onlyActive, + long bndId, + void* callbackHandle, + void (*use)(void* handle, const celix_bundle_t* bnd)); /** * @brief Check whether a bundle is installed. diff --git a/libs/framework/src/dm_dependency_manager_impl.c b/libs/framework/src/dm_dependency_manager_impl.c index 21a77d45..006f97d2 100644 --- a/libs/framework/src/dm_dependency_manager_impl.c +++ b/libs/framework/src/dm_dependency_manager_impl.c @@ -182,10 +182,6 @@ celix_status_t celix_dependencyManager_removeAllComponentsAsync(celix_dependency static void celix_dm_getInfoCallback(void *handle, const celix_bundle_t *bnd) { celix_dependency_manager_info_t **out = handle; - if (celix_bundle_getState(bnd) != CELIX_BUNDLE_STATE_ACTIVE) { - return; - } - celix_bundle_context_t *context = NULL; bundle_getContext((celix_bundle_t*)bnd, &context); celix_dependency_manager_t *mng = celix_bundleContext_getDependencyManager(context); @@ -211,18 +207,15 @@ static void celix_dm_getInfoCallback(void *handle, const celix_bundle_t *bnd) { } celix_dependency_manager_info_t* celix_dependencyManager_createInfo(celix_dependency_manager_t *manager, long bndId) { - celix_dependency_manager_info_t *info = NULL; - celix_bundleContext_useBundle(manager->ctx, bndId, &info, celix_dm_getInfoCallback); - return info; + celix_dependency_manager_info_t* info = NULL; + celix_framework_useBundle( + celix_bundleContext_getFramework(manager->ctx), true /*onlyActive*/, bndId, &info, celix_dm_getInfoCallback); + return info; } static void celix_dm_getInfosCallback(void* handle, const celix_bundle_t* bnd) { celix_array_list_t* infos = handle; - if (celix_bundle_getState(bnd) != CELIX_BUNDLE_STATE_ACTIVE) { - return; - } - celix_bundle_context_t* context = NULL; bundle_getContext((celix_bundle_t*)bnd, &context); celix_dependency_manager_t* mng = celix_bundleContext_getDependencyManager(context); @@ -253,14 +246,14 @@ static void celix_dependencyManager_destroyInfoCallback(void *data, celix_array_ } celix_array_list_t * celix_dependencyManager_createInfos(celix_dependency_manager_t* manager) { - celix_array_list_create_options_t opts = CELIX_EMPTY_ARRAY_LIST_CREATE_OPTIONS; - opts.removedCallbackData = manager; - opts.removedCallback = celix_dependencyManager_destroyInfoCallback; + celix_array_list_create_options_t opts = CELIX_EMPTY_ARRAY_LIST_CREATE_OPTIONS; + opts.removedCallbackData = manager; + opts.removedCallback = celix_dependencyManager_destroyInfoCallback; celix_array_list_t* infos = celix_arrayList_createWithOptions(&opts); - celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx); - celix_framework_useBundles(fw, true, infos, celix_dm_getInfosCallback); - return infos; + celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx); + celix_framework_useActiveBundles(fw, true, infos, celix_dm_getInfosCallback); + return infos; } static void celix_dm_allComponentsActiveCallback(void *handle, const celix_bundle_t *bnd) { diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c index 8671abac..0c6c31b8 100644 --- a/libs/framework/src/framework.c +++ b/libs/framework/src/framework.c @@ -1513,8 +1513,7 @@ static celix_status_t frameworkActivator_destroy(void * userData, bundle_context ********************************************************************************************************************** **********************************************************************************************************************/ - -size_t celix_framework_useBundles(framework_t *fw, bool includeFrameworkBundle, void *callbackHandle, void(*use)(void *handle, const bundle_t *bnd)) { +static size_t celix_framework_useBundlesInternal(framework_t *fw, bool includeFrameworkBundle, bool onlyActive, void *callbackHandle, void(*use)(void *handle, const bundle_t *bnd)) { size_t count = 0; celix_array_list_t *bundleIds = celix_arrayList_create(); @@ -1534,7 +1533,7 @@ size_t celix_framework_useBundles(framework_t *fw, bool includeFrameworkBundle, size = celix_arrayList_size(bundleIds); for (int i = 0; i < size; ++i) { long bndId = celix_arrayList_getLong(bundleIds, i); - bool called = celix_framework_useBundle(fw, false, bndId, callbackHandle, use); + bool called = celix_framework_useBundle(fw, onlyActive, bndId, callbackHandle, use); if (called) { ++count; } @@ -1542,6 +1541,15 @@ size_t celix_framework_useBundles(framework_t *fw, bool includeFrameworkBundle, celix_arrayList_destroy(bundleIds); return count; + +} + +size_t celix_framework_useBundles(framework_t *fw, bool includeFrameworkBundle, void *callbackHandle, void(*use)(void *handle, const bundle_t *bnd)) { + return celix_framework_useBundlesInternal(fw, includeFrameworkBundle, false, callbackHandle, use); +} + +size_t celix_framework_useActiveBundles(framework_t *fw, bool includeFrameworkBundle, void *callbackHandle, void(*use)(void *handle, const bundle_t *bnd)) { + return celix_framework_useBundlesInternal(fw, includeFrameworkBundle, true, callbackHandle, use); } bool celix_framework_useBundle(framework_t *fw, bool onlyActive, long bundleId, void *callbackHandle, void(*use)(void *handle, const bundle_t *bnd)) { @@ -1551,7 +1559,9 @@ bool celix_framework_useBundle(framework_t *fw, bool onlyActive, long bundleId, bundleId); if (entry != NULL) { - celixThreadRwlock_readLock(&entry->fsmMutex); + if (onlyActive) { + celixThreadRwlock_readLock(&entry->fsmMutex); + } celix_bundle_state_e bndState = celix_bundle_getState(entry->bnd); if (onlyActive && (bndState == CELIX_BUNDLE_STATE_ACTIVE || bndState == CELIX_BUNDLE_STATE_STARTING)) { use(callbackHandle, entry->bnd); @@ -1560,7 +1570,9 @@ bool celix_framework_useBundle(framework_t *fw, bool onlyActive, long bundleId, use(callbackHandle, entry->bnd); called = true; } - celixThreadRwlock_unlock(&entry->fsmMutex); + if (onlyActive) { + celixThreadRwlock_unlock(&entry->fsmMutex); + } celix_framework_bundleEntry_decreaseUseCount(entry); } else { framework_logIfError(fw->logger, CELIX_FRAMEWORK_EXCEPTION, NULL, "Bundle with id %li is not installed", bundleId);
