From commits-return-5470-apmail-celix-commits-archive=celix.apache.org@celix.apache.org Thu Sep 12 18:30:48 2019 Return-Path: X-Original-To: apmail-celix-commits-archive@www.apache.org Delivered-To: apmail-celix-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by minotaur.apache.org (Postfix) with SMTP id 2B7F5194BF for ; Thu, 12 Sep 2019 18:30:48 +0000 (UTC) Received: (qmail 71196 invoked by uid 500); 12 Sep 2019 18:30:47 -0000 Delivered-To: apmail-celix-commits-archive@celix.apache.org Received: (qmail 71173 invoked by uid 500); 12 Sep 2019 18:30:47 -0000 Mailing-List: contact commits-help@celix.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@celix.apache.org Delivered-To: mailing list commits@celix.apache.org Received: (qmail 71164 invoked by uid 99); 12 Sep 2019 18:30:47 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 12 Sep 2019 18:30:47 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 47EFF81590; Thu, 12 Sep 2019 18:30:47 +0000 (UTC) Date: Thu, 12 Sep 2019 18:30:48 +0000 To: "commits@celix.apache.org" Subject: [celix] 01/01: Removes some TODO and moves the hanling of listener hooks to service registry MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit From: pnoltes@apache.org In-Reply-To: <156831304720.27801.18260043165186023212@gitbox.apache.org> References: <156831304720.27801.18260043165186023212@gitbox.apache.org> X-Git-Host: gitbox.apache.org X-Git-Repo: celix X-Git-Refname: refs/heads/feature/handle_hooks_in_registry X-Git-Reftype: branch X-Git-Rev: 8751e29bc7b8482e476ba56b589b66e9f22f92a0 X-Git-NotificationType: diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated Message-Id: <20190912183047.47EFF81590@gitbox.apache.org> This is an automated email from the ASF dual-hosted git repository. pnoltes pushed a commit to branch feature/handle_hooks_in_registry in repository https://gitbox.apache.org/repos/asf/celix.git commit 8751e29bc7b8482e476ba56b589b66e9f22f92a0 Author: Pepijn Noltes AuthorDate: Thu Sep 12 20:30:08 2019 +0200 Removes some TODO and moves the hanling of listener hooks to service registry --- libs/framework/include/service_registry.h | 4 +- .../framework/private/mock/service_registry_mock.c | 14 +++- .../private/test/service_registry_test.cpp | 17 +---- libs/framework/src/bundle.c | 3 - libs/framework/src/bundle_context.c | 6 +- libs/framework/src/bundle_revision.c | 2 - libs/framework/src/dm_event.c | 1 - libs/framework/src/framework.c | 89 +++------------------- libs/framework/src/service_registry.c | 75 +++++++++++------- 9 files changed, 77 insertions(+), 134 deletions(-) diff --git a/libs/framework/include/service_registry.h b/libs/framework/include/service_registry.h index 81bbe3c..9150c85 100644 --- a/libs/framework/include/service_registry.h +++ b/libs/framework/include/service_registry.h @@ -93,7 +93,9 @@ serviceRegistry_ungetService(service_registry_pt registry, celix_bundle_t *bundl celix_status_t serviceRegistry_clearReferencesFor(service_registry_pt registry, celix_bundle_t *bundle); -celix_status_t serviceRegistry_getListenerHooks(service_registry_pt registry, celix_bundle_t *bundle, celix_array_list_t **hooks); +void serviceRegistry_callHooksForListenerFilter(service_registry_pt registry, celix_bundle_t *owner, const char *filter, bool removed); + +size_t serviceRegistry_nrOfHooks(service_registry_pt registry); celix_status_t serviceRegistry_servicePropertiesModified(service_registry_pt registry, service_registration_pt registration, diff --git a/libs/framework/private/mock/service_registry_mock.c b/libs/framework/private/mock/service_registry_mock.c index 9942818..52f64a2 100644 --- a/libs/framework/private/mock/service_registry_mock.c +++ b/libs/framework/private/mock/service_registry_mock.c @@ -181,4 +181,16 @@ celix_serviceRegistry_registerServiceFactory( ->withPointerParameters("props", props) ->withOutputParameter("registration", registration); return mock_c()->returnValue().value.intValue; -} \ No newline at end of file +} + +void serviceRegistry_callHooksForListenerFilter( + service_registry_pt registry, + celix_bundle_t *owner, + const char *filter, + bool removed) { + mock_c()->actualCall("serviceRegistry_callHooksForListenerFilter") + ->withPointerParameters("registry", registry) + ->withPointerParameters("owner", owner) + ->withStringParameters("filter", filter) + ->withBoolParameters("removed", removed); +} diff --git a/libs/framework/private/test/service_registry_test.cpp b/libs/framework/private/test/service_registry_test.cpp index f0a4573..cccbd0a 100644 --- a/libs/framework/private/test/service_registry_test.cpp +++ b/libs/framework/private/test/service_registry_test.cpp @@ -1003,23 +1003,10 @@ TEST(service_registry, getListenerHooks) { hashMap_put(usages, (void*)registration->serviceId, reference); hashMap_put(registry->serviceReferences, bundle, usages); - mock() - .expectOneCall("serviceRegistration_retain") - .withParameter("registration", registration); - mock() - .expectOneCall("serviceReference_retain") - .withParameter("ref", reference); - mock() - .expectOneCall("serviceRegistration_release") - .withParameter("registration", registration); - - array_list_pt hooks = NULL; - serviceRegistry_getListenerHooks(registry, bundle, &hooks); - LONGS_EQUAL(1, arrayList_size(hooks)); - POINTERS_EQUAL(reference, arrayList_get(hooks, 0)); + size_t nrOfHooks = serviceRegistry_nrOfHooks(registry); + LONGS_EQUAL(1, nrOfHooks); hashMap_destroy(usages, false, false); - arrayList_destroy(hooks); arrayList_remove(registry->listenerHooks, 0); free(registration); serviceRegistry_destroy(registry); diff --git a/libs/framework/src/bundle.c b/libs/framework/src/bundle.c index cd2610a..f05beae 100644 --- a/libs/framework/src/bundle.c +++ b/libs/framework/src/bundle.c @@ -274,7 +274,6 @@ celix_status_t bundle_update(bundle_pt bundle, const char *inputFile) { status = bundle_isSystemBundle(bundle, &systemBundle); if (status == CELIX_SUCCESS) { if (systemBundle) { - // #TODO: Support framework update status = CELIX_BUNDLE_EXCEPTION; } else { status = framework_updateBundle(bundle->framework, bundle, inputFile); @@ -461,8 +460,6 @@ celix_status_t bundle_closeAndDelete(bundle_pt bundle) { celix_status_t bundle_closeRevisions(bundle_pt bundle) { celix_status_t status = CELIX_SUCCESS; - - // TODO implement this return status; } diff --git a/libs/framework/src/bundle_context.c b/libs/framework/src/bundle_context.c index 5f78f85..506fb1e 100644 --- a/libs/framework/src/bundle_context.c +++ b/libs/framework/src/bundle_context.c @@ -84,8 +84,6 @@ celix_status_t bundleContext_destroy(bundle_context_pt context) { bundleContext_cleanupServiceTrackers(context); bundleContext_cleanupServiceTrackerTrackers(context); - //TODO cleanup service registrations - //NOTE still present service registrations will be cleared during bundle stop in the //service registry (serviceRegistry_clearServiceRegistrations). celixThreadMutex_unlock(&context->mutex); @@ -572,8 +570,6 @@ long celix_bundleContext_trackBundlesWithOptions( trackerId = entry->trackerId; //loop through all already installed bundles. - // FIXME there is a race condition between installing the listener and looping through the started bundles. - // NOTE move this to the framework, so that the framework can ensure locking to ensure not bundles is missed. if (entry->opts.onStarted != NULL) { celix_framework_useBundles(ctx->framework, entry->opts.includeFrameworkBundle, entry->opts.callbackHandle, entry->opts.onStarted); } @@ -758,7 +754,7 @@ bool celix_bundleContext_stopBundle(celix_bundle_context_t *ctx, long bundleId) static void bundleContext_uninstallBundleCallback(void *handle, const bundle_t *c_bnd) { bool *uninstalled = handle; - bundle_t *bnd = (bundle_t*)c_bnd; //TODO use mute-able variant ?? + bundle_t *bnd = (bundle_t*)c_bnd; if (celix_bundle_getState(bnd) == OSGI_FRAMEWORK_BUNDLE_ACTIVE) { bundle_stop(bnd); } diff --git a/libs/framework/src/bundle_revision.c b/libs/framework/src/bundle_revision.c index 9165c7d..2f4d2f1 100644 --- a/libs/framework/src/bundle_revision.c +++ b/libs/framework/src/bundle_revision.c @@ -42,7 +42,6 @@ celix_status_t bundleRevision_create(const char *root, const char *location, lon if (!revision) { status = CELIX_ENOMEM; } else { - // TODO: This overwrites an existing revision, is this supposed to happen? int state = mkdir(root, S_IRWXU); if ((state != 0) && (errno != EEXIST)) { free(revision); @@ -51,7 +50,6 @@ celix_status_t bundleRevision_create(const char *root, const char *location, lon if (inputFile != NULL) { status = extractBundle(inputFile, root); } else if (strcmp(location, "inputstream:") != 0) { - // TODO how to handle this correctly? // If location != inputstream, extract it, else ignore it and assume this is a cache entry. status = extractBundle(location, root); } diff --git a/libs/framework/src/dm_event.c b/libs/framework/src/dm_event.c index 6375c4d..c1cc4ba 100644 --- a/libs/framework/src/dm_event.c +++ b/libs/framework/src/dm_event.c @@ -43,7 +43,6 @@ celix_status_t event_create(dm_event_type_e event_type, bundle_pt bundle, bundle serviceReference_getProperty(reference, OSGI_FRAMEWORK_SERVICE_ID, &serviceIdStr); unsigned long servId = strtoul(serviceIdStr,NULL,10); - //FIXME service ranking can dynamically change, but service reference can be removed at any time. const char* rankingStr = NULL; serviceReference_getProperty(reference, OSGI_FRAMEWORK_SERVICE_RANKING, &rankingStr); long ranking = rankingStr == NULL ? 0 : atol(rankingStr); diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c index b15dde4..51615a7 100644 --- a/libs/framework/src/framework.c +++ b/libs/framework/src/framework.c @@ -231,7 +231,6 @@ struct request { typedef struct request *request_pt; -//TODO move to service registry typedef struct celix_fw_service_listener_entry { //only set during creating celix_bundle_t *bundle; @@ -301,7 +300,6 @@ static inline void listener_waitAndDestroy(celix_framework_t *framework, celix_f framework_logger_pt logger; -//TODO introduce a counter + mutex to control the freeing of the logger when mutiple threads are running a framework. static celix_thread_once_t loggerInit = CELIX_THREAD_ONCE_INIT; static void framework_loggerInit(void) { logger = malloc(sizeof(*logger)); @@ -324,11 +322,11 @@ celix_status_t framework_create(framework_pt *framework, properties_pt config) { status = CELIX_DO_IF(status, celixThreadCondition_init(&(*framework)->shutdown.cond, NULL)); status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->shutdown.mutex, NULL)); - status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->dispatcher.mutex, NULL)); //TODO refactor to use use count with condition (see serviceListeners) + status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->dispatcher.mutex, NULL)); status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->serviceListenersLock, &attr)); - status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->frameworkListenersLock, &attr)); //TODO refactor to use use count with condition (see serviceListeners) - status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->bundleListenerLock, NULL)); //TODO refactor to use use count with condition (see serviceListeners) - status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->installedBundles.mutex, NULL)); //TODO refactor to use use count with condition (see serviceListeners) + status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->frameworkListenersLock, &attr)); + status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->bundleListenerLock, NULL)); + status = CELIX_DO_IF(status, celixThreadMutex_create(&(*framework)->installedBundles.mutex, NULL)); status = CELIX_DO_IF(status, celixThreadCondition_init(&(*framework)->dispatcher.cond, NULL)); if (status == CELIX_SUCCESS) { (*framework)->bundle = NULL; @@ -352,8 +350,7 @@ celix_status_t framework_create(framework_pt *framework, properties_pt config) { status = CELIX_DO_IF(status, bundle_getBundleId((*framework)->bundle, &(*framework)->bundleId)); status = CELIX_DO_IF(status, bundle_setFramework((*framework)->bundle, (*framework))); if (status == CELIX_SUCCESS) { - // - //TODO set group (*framework)->bundle + //nop } else { status = CELIX_FRAMEWORK_EXCEPTION; fw_logCode((*framework)->logger, OSGI_FRAMEWORK_LOG_ERROR, status, "Could not create framework"); @@ -656,7 +653,6 @@ static void framework_autoStartConfiguredBundles(bundle_context_t *fwCtx) { for (int i = 0; i < len; ++i) { bundleContext_getProperty(fwCtx, celixKeys[i], &autoStart); if (autoStart == NULL) { - //trying cosgi.auto.start. variants. TODO make this prop deprecated -> warning bundleContext_getProperty(fwCtx, cosgiKeys[i], &autoStart); } if (autoStart != NULL) { @@ -780,7 +776,7 @@ celix_status_t fw_installBundle2(framework_pt framework, bundle_pt * bundle, lon return CELIX_FILE_IO_EXCEPTION; } - //increase use count of framework bundle to prevent a stop. TODO is concurrent installing of bundles supported? -> else need another lock + //increase use count of framework bundle to prevent a stop. fw_bundleEntry_increaseUseCount(framework, framework->bundleId); status = CELIX_DO_IF(status, bundle_getState(framework->bundle, &state)); @@ -1158,7 +1154,6 @@ celix_status_t fw_stopBundle(framework_pt framework, bundle_pt bundle, bool reco serviceRegistry_clearReferencesFor(framework->registry, bundle); } - // #TODO remove listeners for bundle if (context != NULL) { status = CELIX_DO_IF(status, bundleContext_destroy(context)); @@ -1230,7 +1225,6 @@ celix_status_t fw_uninstallBundle(framework_pt framework, bundle_pt bundle) { status = CELIX_DO_IF(status, fw_stopBundle(framework, bundle, true)); - // TODO Unload all libraries for transition to unresolved bundle_archive_t *archive = NULL; bundle_revision_t *revision = NULL; array_list_pt handles = NULL; @@ -1623,50 +1617,13 @@ celix_status_t framework_ungetService(framework_pt framework, bundle_pt bundle, } void fw_addServiceListener(framework_pt framework, bundle_pt bundle, celix_service_listener_t *listener, const char* sfilter) { - array_list_pt listenerHooks = NULL; - unsigned int i; - celix_fw_service_listener_entry_t *fwListener = listener_create(bundle, sfilter, listener); - bundle_context_t *context = NULL; - celixThreadMutex_lock(&framework->serviceListenersLock); arrayList_add(framework->serviceListeners, fwListener); celixThreadMutex_unlock(&framework->serviceListenersLock); - //TODO lock listeners hooks? - celix_status_t status = serviceRegistry_getListenerHooks(framework->registry, framework->bundle, &listenerHooks); - - if (status == CELIX_SUCCESS) { - struct listener_hook_info info; - - bundle_getContext(bundle, &context); - info.context = context; - info.removed = false; - info.filter = sfilter; - - for (i = 0; i < arrayList_size(listenerHooks); i++) { - service_reference_pt ref = (service_reference_pt) arrayList_get(listenerHooks, i); - listener_hook_service_pt hook = NULL; - array_list_pt infos = NULL; - bool ungetResult = false; - - status = fw_getService(framework, framework->bundle, ref, (const void **) &hook); - - if (status == CELIX_SUCCESS && hook != NULL) { - arrayList_create(&infos); - arrayList_add(infos, &info); - hook->added(hook->handle, infos); - serviceRegistry_ungetService(framework->registry, framework->bundle, ref, &ungetResult); - serviceRegistry_ungetServiceReference(framework->registry, framework->bundle, ref); - arrayList_destroy(infos); - } else { - fw_logCode(framework->logger, OSGI_FRAMEWORK_LOG_ERROR, status, "Could not retrieve hook service."); - } - } - - arrayList_destroy(listenerHooks); - } + serviceRegistry_callHooksForListenerFilter(framework->registry, bundle, sfilter, false); } void fw_removeServiceListener(framework_pt framework, bundle_pt bundle, celix_service_listener_t *listener) { @@ -1690,35 +1647,9 @@ void fw_removeServiceListener(framework_pt framework, bundle_pt bundle, celix_se if (match != NULL) { //invoke listener hooks - - bundle_context_pt lContext = NULL; - - struct listener_hook_info info; - bundle_getContext(match->bundle, &lContext); - info.context = lContext; - filter_getString(match->filter, &info.filter); - info.removed = true; - - array_list_pt listenerHooks = NULL; - serviceRegistry_getListenerHooks(framework->registry, framework->bundle, &listenerHooks); - for (i = 0; i < arrayList_size(listenerHooks); i++) { - service_reference_pt ref = (service_reference_pt) arrayList_get(listenerHooks, i); - listener_hook_service_pt hook = NULL; - array_list_pt infos = NULL; - bool ungetResult; - - fw_getService(framework, framework->bundle, ref, (const void **) &hook); - - if (hook != NULL) { - arrayList_create(&infos); - arrayList_add(infos, &info); - hook->removed(hook->handle, infos); - serviceRegistry_ungetService(framework->registry, framework->bundle, ref, &ungetResult); - serviceRegistry_ungetServiceReference(framework->registry, framework->bundle, ref); - arrayList_destroy(infos); - } - } - arrayList_destroy(listenerHooks); + const char *filter; + filter_getString(match->filter, &filter); + serviceRegistry_callHooksForListenerFilter(framework->registry, bundle, filter, true); } if (match != NULL) { diff --git a/libs/framework/src/service_registry.c b/libs/framework/src/service_registry.c index 5d2d6c5..1d8bd2b 100644 --- a/libs/framework/src/service_registry.c +++ b/libs/framework/src/service_registry.c @@ -760,42 +760,63 @@ static celix_status_t serviceRegistry_removeHook(service_registry_pt registry, s return status; } -celix_status_t serviceRegistry_getListenerHooks(service_registry_pt registry, bundle_pt owner, array_list_pt *out) { - celix_status_t status; - array_list_pt result; +void serviceRegistry_callHooksForListenerFilter(service_registry_pt registry, celix_bundle_t *owner, const char *filter, bool removed) { + celix_bundle_context_t *ctx; + bundle_getContext(owner, &ctx); - status = arrayList_create(&result); - if (status == CELIX_SUCCESS) { - unsigned int i; - unsigned size = arrayList_size(registry->listenerHooks); + struct listener_hook_info info; + info.context = ctx; + info.removed = removed; + info.filter = filter; + celix_array_list_t *infos = celix_arrayList_create(); + celix_arrayList_add(infos, &info); - for (i = 0; i < size; i += 1) { - celixThreadRwlock_readLock(®istry->lock); - service_registration_pt registration = arrayList_get(registry->listenerHooks, i); - if (registration != NULL) { - serviceRegistration_retain(registration); - } - celixThreadRwlock_unlock(®istry->lock); + celix_array_list_t *hookRegistrations = celix_arrayList_create(); - if (registration != NULL) { - service_reference_pt reference = NULL; - serviceRegistry_getServiceReference(registry, owner, registration, &reference); - arrayList_add(result, reference); - serviceRegistration_release(registration); - } + celixThreadRwlock_readLock(®istry->lock); + unsigned size = arrayList_size(registry->listenerHooks); + for (int i = 0; i < size; ++i) { + service_registration_pt registration = arrayList_get(registry->listenerHooks, i); + if (registration != NULL) { + serviceRegistration_retain(registration); + celix_arrayList_add(hookRegistrations, registration); } } + celixThreadRwlock_unlock(®istry->lock); - if (status == CELIX_SUCCESS) { - *out = result; - } else { - if (result != NULL) { - arrayList_destroy(result); + for (int i = 0; i < arrayList_size(hookRegistrations); ++i) { + service_registration_pt reg = celix_arrayList_get(hookRegistrations, i); + service_reference_pt ref; + serviceRegistry_getServiceReference(registry, owner, reg, &ref); + + listener_hook_service_pt hook = NULL; + celix_status_t status = serviceRegistry_getService(registry, owner, ref, (const void **) &hook); + + if (status == CELIX_SUCCESS && hook != NULL) { + if (removed) { + hook->removed(hook->handle, infos); + } else { + hook->added(hook->handle, infos); + } + + bool ungetResult = false; + serviceRegistry_ungetService(registry, owner, ref, &ungetResult); + serviceRegistry_ungetServiceReference(registry, owner, ref); + } else { + fw_logCode(logger, OSGI_FRAMEWORK_LOG_ERROR, status, "Could not retrieve hook service."); } - framework_logIfError(logger, status, NULL, "Cannot get listener hooks"); + + serviceRegistration_release(reg); } + celix_arrayList_destroy(hookRegistrations); + celix_arrayList_destroy(infos); +} - return status; +size_t serviceRegistry_nrOfHooks(service_registry_pt registry) { + celixThreadRwlock_readLock(®istry->lock); + unsigned size = arrayList_size(registry->listenerHooks); + celixThreadRwlock_unlock(®istry->lock); + return (size_t) size; } celix_status_t serviceRegistry_servicePropertiesModified(service_registry_pt registry, service_registration_pt registration, properties_pt oldprops) {