Skip to content

Commit

Permalink
CELIX-466: Adds a thread safe use count check on service listener hooks.
Browse files Browse the repository at this point in the history
  • Loading branch information
@pnoltes
pnoltes committed Sep 16, 2019
1 parent 8751e29 commit e12817a
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 58 deletions.
31 changes: 18 additions & 13 deletions libs/framework/include/listener_hook_service.h
View file
Expand Up @@ -16,28 +16,23 @@
*specific language governing permissions and limitations
*under the License.
*/
/*
* listener_hook_service.h
*
* \date Oct 28, 2011
* \author <a href="mailto:dev@celix.apache.org">Apache Celix Project Team</a>
* \copyright Apache License, Version 2.0
*/

#ifndef LISTENER_HOOK_SERVICE_H_
#define LISTENER_HOOK_SERVICE_H_


typedef struct listener_hook_info *listener_hook_info_pt;
typedef struct listener_hook_service *listener_hook_service_pt;

#include "bundle_context.h"

#define OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME "listener_hook_service"
#ifdef __cplusplus
extern "C" {
#endif


typedef struct listener_hook_info *listener_hook_info_pt;
typedef struct listener_hook_info celix_listener_hook_info_t;
typedef struct listener_hook_service *listener_hook_service_pt;
typedef struct listener_hook_service celix_listener_hook_service_t;

struct listener_hook_info {
celix_bundle_context_t *context;
const char *filter;
Expand All @@ -47,9 +42,19 @@ struct listener_hook_info {
struct listener_hook_service {
void *handle;

celix_status_t (*added)(void *hook, celix_array_list_t *listeners);
/**
* Called when a new service listener / service tracker is added.
* @param handle The service handle.
* @param listeners A list of celix_listener_hook_info_t* entries.
*/
celix_status_t (*added)(void *handle, celix_array_list_t *listeners);

celix_status_t (*removed)(void *hook, celix_array_list_t *listeners);
/**
* Called when a service listener / service tracker is removed.
* @param handle The service handle.
* @param listeners A list of celix_listener_hook_info_t* entries.
*/
celix_status_t (*removed)(void *handle, celix_array_list_t *listeners);
};

#ifdef __cplusplus
Expand Down
15 changes: 14 additions & 1 deletion libs/framework/private/test/service_registry_test.cpp
View file
Expand Up @@ -264,6 +264,7 @@ TEST(service_registry, registerServiceListenerHook) {
char * serviceName = my_strdup(OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME);
void *service = (void *) 0x20;
service_registration_pt reg = (service_registration_pt) 0x30;
long svcId = 11;

mock()
.expectOneCall("serviceRegistration_create")
Expand All @@ -281,11 +282,17 @@ TEST(service_registry, registerServiceListenerHook) {
.withParameter("registration", reg)
.withParameter("oldprops", (void*)NULL);

mock().expectOneCall("serviceRegistration_getServiceId")
.withParameter("registration", reg)
.andReturnValue(svcId);

service_registration_pt registration = NULL;
serviceRegistry_registerService(registry, bundle, serviceName, service, NULL, &registration);
POINTERS_EQUAL(reg, registration);
LONGS_EQUAL(1, arrayList_size(registry->listenerHooks));
POINTERS_EQUAL(reg, arrayList_get(registry->listenerHooks, 0));

auto* entry = (celix_service_registry_listener_hook_entry_t*)celix_arrayList_get(registry->listenerHooks, 0);
POINTERS_EQUAL(svcId, entry->svcId);

//cleanup
array_list_pt destroy_this = (array_list_pt) hashMap_remove(registry->serviceRegistrations, bundle);
Expand Down Expand Up @@ -313,6 +320,7 @@ TEST(service_registry, unregisterService) {
hashMap_put(references, (void*)registration->serviceId, reference);
hashMap_put(registry->serviceReferences, bundle, references);
properties_pt properties = (properties_pt) 0x40;
long svcId = 12;

mock()
.expectOneCall("serviceRegistration_getProperties")
Expand All @@ -325,12 +333,17 @@ TEST(service_registry, unregisterService) {
.withParameter("key", (char *)OSGI_FRAMEWORK_OBJECTCLASS)
.andReturnValue((char*)OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME);

mock().expectOneCall("serviceRegistration_getServiceId")
.withParameter("registration", registration)
.andReturnValue(svcId);

mock()
.expectOneCall("serviceRegistryTest_serviceChanged")
.withParameter("framework", framework)
.withParameter("eventType", OSGI_FRAMEWORK_SERVICE_EVENT_UNREGISTERING)
.withParameter("registration", registration)
.withParameter("oldprops", (void*) NULL);

mock()
.expectOneCall("serviceReference_invalidate")
.withParameter("reference", reference);
Expand Down
7 changes: 0 additions & 7 deletions libs/framework/src/bundle_context_private.h
View file
Expand Up @@ -16,13 +16,6 @@
*specific language governing permissions and limitations
*under the License.
*/
/*
* bundle_context_private.h
*
* \date Feb 12, 2013
* \author <a href="mailto:dev@celix.apache.org">Apache Celix Project Team</a>
* \copyright Apache License, Version 2.0
*/


#ifndef BUNDLE_CONTEXT_PRIVATE_H_
Expand Down
121 changes: 85 additions & 36 deletions libs/framework/src/service_registry.c
View file
Expand Up @@ -16,13 +16,7 @@
*specific language governing permissions and limitations
*under the License.
*/
/*
* service_registry.c
*
* \date Aug 6, 2010
* \author <a href="mailto:dev@celix.apache.org">Apache Celix Project Team</a>
* \copyright Apache License, Version 2.0
*/

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
Expand Down Expand Up @@ -55,6 +49,11 @@ static celix_status_t serviceRegistry_setReferenceStatus(service_registry_pt reg
static celix_status_t serviceRegistry_getUsingBundles(service_registry_pt registry, service_registration_pt reg, array_list_pt *bundles);
static celix_status_t serviceRegistry_getServiceReference_internal(service_registry_pt registry, bundle_pt owner, service_registration_pt registration, service_reference_pt *out);

static celix_service_registry_listener_hook_entry_t* celix_createHookEntry(long svcId, celix_listener_hook_service_t*);
static void celix_waitAndDestroyHookEntry(celix_service_registry_listener_hook_entry_t *entry);
static void celix_increaseCountHook(celix_service_registry_listener_hook_entry_t *entry);
static void celix_decreaseCountHook(celix_service_registry_listener_hook_entry_t *entry);

celix_status_t serviceRegistry_create(framework_pt framework, serviceChanged_function_pt serviceChanged, service_registry_pt *out) {
celix_status_t status;

Expand Down Expand Up @@ -127,9 +126,12 @@ celix_status_t serviceRegistry_destroy(service_registry_pt registry) {
hashMap_destroy(registry->serviceReferences, false, false);

//destroy listener hooks
size = arrayList_size(registry->listenerHooks);
if (size == 0)
arrayList_destroy(registry->listenerHooks);
size = celix_arrayList_size(registry->listenerHooks);
for (int i = 0; i < celix_arrayList_size(registry->listenerHooks); ++i) {
celix_service_registry_listener_hook_entry_t *entry = celix_arrayList_get(registry->listenerHooks, i);
celix_waitAndDestroyHookEntry(entry);
}
celix_arrayList_destroy(registry->listenerHooks);

hashMap_destroy(registry->deletedServiceReferences, false, false);

Expand Down Expand Up @@ -737,7 +739,9 @@ static celix_status_t serviceRegistry_addHooks(service_registry_pt registry, con

if (strcmp(OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME, serviceName) == 0) {
celixThreadRwlock_writeLock(&registry->lock);
arrayList_add(registry->listenerHooks, registration);
long svcId = serviceRegistration_getServiceId(registration);
celix_service_registry_listener_hook_entry_t *entry = celix_createHookEntry(svcId, (celix_listener_hook_service_t*)serviceObject);
arrayList_add(registry->listenerHooks, entry);
celixThreadRwlock_unlock(&registry->lock);
}

Expand All @@ -748,15 +752,29 @@ static celix_status_t serviceRegistry_removeHook(service_registry_pt registry, s
celix_status_t status = CELIX_SUCCESS;
const char* serviceName = NULL;

celix_service_registry_listener_hook_entry_t *removedEntry = NULL;

properties_pt props = NULL;
serviceRegistration_getProperties(registration, &props);
serviceName = properties_get(props, (char *) OSGI_FRAMEWORK_OBJECTCLASS);
long svcId = serviceRegistration_getServiceId(registration);
if (strcmp(OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME, serviceName) == 0) {
celixThreadRwlock_writeLock(&registry->lock);
arrayList_removeElement(registry->listenerHooks, registration);
for (int i = 0; i < celix_arrayList_size(registry->listenerHooks); ++i) {
celix_service_registry_listener_hook_entry_t *visit = celix_arrayList_get(registry->listenerHooks, i);
if (visit->svcId == svcId) {
removedEntry = visit;
celix_arrayList_removeAt(registry->listenerHooks, i);
break;
}
}
celixThreadRwlock_unlock(&registry->lock);
}

if (removedEntry != NULL) {
celix_waitAndDestroyHookEntry(removedEntry);
}

return status;
}

Expand All @@ -776,37 +794,22 @@ void serviceRegistry_callHooksForListenerFilter(service_registry_pt registry, ce
celixThreadRwlock_readLock(&registry->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);
celix_service_registry_listener_hook_entry_t* entry = arrayList_get(registry->listenerHooks, i);
if (entry != NULL) {
celix_increaseCountHook(entry); //increate use count to ensure the hook cannot be removed, untill used
celix_arrayList_add(hookRegistrations, entry);
}
}
celixThreadRwlock_unlock(&registry->lock);

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);
celix_service_registry_listener_hook_entry_t* entry = celix_arrayList_get(hookRegistrations, i);
if (removed) {
entry->hook->removed(entry->hook->handle, infos);
} else {
fw_logCode(logger, OSGI_FRAMEWORK_LOG_ERROR, status, "Could not retrieve hook service.");
entry->hook->added(entry->hook->handle, infos);
}

serviceRegistration_release(reg);
celix_decreaseCountHook(entry); //done using hook. decrease count
}
celix_arrayList_destroy(hookRegistrations);
celix_arrayList_destroy(infos);
Expand Down Expand Up @@ -868,3 +871,49 @@ celix_serviceRegistry_registerServiceFactory(
service_registration_t **registration) {
return serviceRegistry_registerServiceInternal(reg, (celix_bundle_t*)bnd, serviceName, (const void *) factory, props, CELIX_FACTORY_SERVICE, registration);
}

static celix_service_registry_listener_hook_entry_t* celix_createHookEntry(long svcId, celix_listener_hook_service_t *hook) {
celix_service_registry_listener_hook_entry_t* entry = calloc(1, sizeof(*entry));
entry->svcId = svcId;
entry->hook = hook;
celixThreadMutex_create(&entry->mutex, NULL);
celixThreadCondition_init(&entry->cond, NULL);
return entry;
}

static void celix_waitAndDestroyHookEntry(celix_service_registry_listener_hook_entry_t *entry) {
if (entry != NULL) {
celixThreadMutex_lock(&entry->mutex);
int waitCount = 0;
while (entry->count > 0) {
celixThreadCondition_timedwaitRelative(&entry->cond, &entry->mutex, 1, 0); //wait for 1 second
waitCount += 1;
if (waitCount >= 5) {
fw_log(logger, OSGI_FRAMEWORK_LOG_WARNING,
"Still waiting for service listener hook use count to become zero. Waiting for %i seconds. Use Count is %i, svc id is %li", waitCount, (int)entry->count, entry->svcId);
}
}
celixThreadMutex_unlock(&entry->mutex);

//done waiting, use count is on 0
celixThreadCondition_destroy(&entry->cond);
celixThreadMutex_destroy(&entry->mutex);
free(entry);
}
}
static void celix_increaseCountHook(celix_service_registry_listener_hook_entry_t *entry) {
if (entry != NULL) {
celixThreadMutex_lock(&entry->mutex);
entry->count += 1;
celixThreadCondition_broadcast(&entry->cond);
celixThreadMutex_unlock(&entry->mutex);
}
}
static void celix_decreaseCountHook(celix_service_registry_listener_hook_entry_t *entry) {
if (entry != NULL) {
celixThreadMutex_lock(&entry->mutex);
entry->count -= 1;
celixThreadCondition_broadcast(&entry->cond);
celixThreadMutex_unlock(&entry->mutex);
}
}
12 changes: 11 additions & 1 deletion libs/framework/src/service_registry_private.h
View file
Expand Up @@ -30,6 +30,8 @@

#include "registry_callback_private.h"
#include "service_registry.h"
#include "listener_hook_service.h"
#include "service_reference.h"

struct celix_serviceRegistry {
framework_pt framework;
Expand All @@ -44,11 +46,19 @@ struct celix_serviceRegistry {
serviceChanged_function_pt serviceChanged;
unsigned long currentServiceId;

array_list_pt listenerHooks;
array_list_pt listenerHooks; //celix_service_registry_listener_hook_entry_t*

celix_thread_rwlock_t lock;
};

typedef struct celix_service_registry_listener_hook_entry {
long svcId;
celix_listener_hook_service_t *hook;
celix_thread_mutex_t mutex; //protects below
celix_thread_cond_t cond;
unsigned int count;
} celix_service_registry_listener_hook_entry_t;

typedef enum reference_status_enum {
REF_ACTIVE,
REF_DELETED,
Expand Down

0 comments on commit e12817a

Please sign in to comment.