Skip to content
Permalink
Browse files
fix thread-safety bug introduced by 691dc6c and broken tracker set ca…
…llback.

One intent of 691dc6c is to make bundleContext_retainServiceReference and bundleContext_retainServiceReference lockless. Unfortunately it introduced a race condition: if a dying reference (still in registry) get revived by another call of bundleContext_getServiceReferences before it's actually removed from the registry and destroyed, hazard happens.

The set callback of an all-service tracker, whose service name is empty, did not work as expected by CelixBundleContextServicesTests.TrackerOfAllServicesSetTest. Unnecessary set callbacks when closing trackers are eliminated.
  • Loading branch information
PengZheng committed Feb 22, 2022
1 parent 898f8a7 commit ef1202fec790759e2e13f64973bc788862c0fdd8
Show file tree
Hide file tree
Showing 13 changed files with 186 additions and 68 deletions.
@@ -623,9 +623,7 @@ TEST_F(CxxBundleContextTestSuite, setServicesWithTrackerWhenMultipleRegistration
tracker->close();
tracker->wait();

//TODO improve this. For now closing a tracker will inject other service before getting to nullptr.
//Also look into why this is not happening in the C service tracker test.
EXPECT_EQ(6, count.load());
EXPECT_EQ(2, count.load());
}

TEST_F(CxxBundleContextTestSuite, WaitForAllEvents) {
@@ -114,6 +114,59 @@ TEST_F(CelixBundleContextServicesTests, incorrectAsyncUnregisterCalls) {
celix_bundleContext_unregisterServiceAsync(ctx, -2, nullptr, nullptr);
};

TEST_F(CelixBundleContextServicesTests, UseServicesWithoutName) {
struct calc {
int (*calc)(int);
};

const char *calcName = "calc";
struct calc svc;
svc.calc = [](int n) -> int {
return n * 42;
};

long svcId1 = celix_bundleContext_registerService(ctx, &svc, calcName, nullptr);
EXPECT_TRUE(svcId1 >= 0);

auto use = [](void *handle, void *svc) {
EXPECT_TRUE(svc != nullptr);
int *total = static_cast<int*>(handle);
struct calc *calc = static_cast<struct calc*>(svc);
int tmp = calc->calc(1);
*total += tmp;
};

int total = 0;
auto count = celix_bundleContext_useServices(ctx, nullptr, &total, use);
EXPECT_EQ(0, count);
count = celix_bundleContext_useServices(ctx, calcName, &total, use);
EXPECT_EQ(1, count);
bool called = celix_bundleContext_useService(ctx, nullptr, &total, use);
EXPECT_FALSE(called);
called = celix_bundleContext_useService(ctx, calcName, &total, use);
EXPECT_TRUE(called);
called = celix_bundleContext_useServiceWithId(ctx, svcId1, nullptr, &total, use);
EXPECT_FALSE(called);

celix_service_use_options_t use_opts{};
use_opts.filter.serviceName = nullptr;
use_opts.filter.versionRange = nullptr;
use_opts.callbackHandle = &total;
use_opts.use = use;
called = celix_bundleContext_useServiceWithOptions(ctx, &use_opts);
ASSERT_FALSE(called);
count = celix_bundleContext_useServicesWithOptions(ctx, &use_opts);
EXPECT_EQ(0, count);

use_opts.filter.serviceName = calcName;
called = celix_bundleContext_useServiceWithOptions(ctx, &use_opts);
ASSERT_TRUE(called);
count = celix_bundleContext_useServicesWithOptions(ctx, &use_opts);
EXPECT_EQ(1, count);

celix_bundleContext_unregisterService(ctx, svcId1);
}

TEST_F(CelixBundleContextServicesTests, registerMultipleAndUseServices) {
struct calc {
int (*calc)(int);
@@ -867,6 +920,64 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerSetTest) {
ASSERT_EQ(4, count); //check if the set is called the expected times
}

TEST_F(CelixBundleContextServicesTests, TrackerOfAllServicesSetTest) {
int count = 0;

void *svc1 = (void*)0x100; //no ranking
void *svc2 = (void*)0x200; //no ranking
void *svc3 = (void*)0x300; //10 ranking
void *svc4 = (void*)0x400; //5 ranking

auto set = [](void *handle, void *svc) {
static int callCount = 0;
callCount += 1;
if (callCount == 1) {
//first time svc1 should be set (oldest service with equal ranking
ASSERT_EQ(0x100, (long)svc);
} else if (callCount == 2) {
ASSERT_EQ(0x300, (long)svc);
//second time svc3 should be set (highest ranking)
} else if (callCount == 3) {
//third time svc4 should be set (highest ranking
ASSERT_EQ(0x400, (long)svc);
}

int *c = static_cast<int*>(handle);
*c = callCount;
};

long svcId1 = celix_bundleContext_registerService(ctx, svc1, "NA", nullptr);
long svcId2 = celix_bundleContext_registerService(ctx, svc2, "NA", nullptr);

//starting tracker should lead to first set call
celix_service_tracking_options_t opts{};
opts.callbackHandle = (void*)&count;
opts.filter.serviceName = nullptr;
opts.set = set;
long trackerId = celix_bundleContext_trackServicesWithOptions(ctx, &opts); //call 1
ASSERT_TRUE(trackerId >= 0);

//register svc3 should lead to second set call
properties_t *props3 = celix_properties_create();
celix_properties_set(props3, OSGI_FRAMEWORK_SERVICE_RANKING, "10");
long svcId3 = celix_bundleContext_registerService(ctx, svc3, "NA", props3); //call 2

//register svc4 should lead to no set (lower ranking)
properties_t *props4 = celix_properties_create();
celix_properties_set(props4, OSGI_FRAMEWORK_SERVICE_RANKING, "10");
long svcId4 = celix_bundleContext_registerService(ctx, svc4, "NA", props4); //no update

//unregister svc3 should lead to set (new highest ranking)
celix_bundleContext_unregisterService(ctx, svcId3); //call 3

celix_bundleContext_stopTracker(ctx, trackerId); //call 4 (nullptr)
celix_bundleContext_unregisterService(ctx, svcId1);
celix_bundleContext_unregisterService(ctx, svcId2);
celix_bundleContext_unregisterService(ctx, svcId4);

ASSERT_EQ(4, count); //check if the set is called the expected times
}

TEST_F(CelixBundleContextServicesTests, trackAllServices) {
std::atomic<size_t> count{0};

@@ -662,7 +662,7 @@ void celix_bundleContext_stopTracker(celix_bundle_context_t *ctx, long trackerId
* the targeted service cannot be removed during the callback.
*
* The svc is should only be considered valid during the callback.
* If no service is found the callback will not be invoked.
* If no service is found, the callback will not be invoked and this function will return false immediately.
*
* This function will block until the callback is finished. As result it is possible to provide callback data from the
* stack.
@@ -688,7 +688,7 @@ bool celix_bundleContext_useServiceWithId(
* The Celix framework will ensure that the targeted service cannot be removed during the callback.
*
* The svc is should only be considered valid during the callback.
* If no service is found the callback will not be invoked.
* If no service is found, the callback will not be invoked and this function will return false immediately.
*
* This function will block until the callback is finished. As result it is possible to provide callback data from the
* stack.
@@ -712,7 +712,7 @@ bool celix_bundleContext_useService(
* The Celix framework will ensure that the targeted service cannot be removed during the callback.
*
* The svc is should only be considered valid during the callback.
* If no service is found the callback will not be invoked.
* If no service is found, the callback will not be invoked and this function will return 0 immediately.
*
* This function will block until the callback is finished. As result it is possible to provide callback data from the
* stack.
@@ -794,7 +794,8 @@ typedef struct celix_service_use_options {
* The Celix framework will ensure that the targeted service cannot be removed during the callback.
*
* The svc is should only be considered valid during the callback.
* If no service is found the callback will not be invoked.
* If no service is found the callback will not be invoked. In such cases, if a non-zero waitTimeoutInSeconds is specified in opts,
* this function will block until the timeout is expired or when at least one service is found, otherwise it will return false immediately.
*
* This function will block until the callback is finished. As result it is possible to provide callback data from the
* stack.
@@ -814,7 +815,8 @@ bool celix_bundleContext_useServiceWithOptions(
* The Celix framework will ensure that the targeted service cannot be removed during the callback.
*
* The svc is should only be considered valid during the callback.
* If no service is found the callback will not be invoked.
* If no service is found, the callback will not be invoked and this function will return 0 immediately.
* Note that waitTimeoutInSeconds in opts has no effect.
*
* This function will block until the callback is finished. As result it is possible to provide callback data from the
* stack.
@@ -65,9 +65,6 @@ celix_status_t
serviceRegistry_getServiceReferences(service_registry_pt registry, celix_bundle_t *bundle, const char *serviceName,
filter_pt filter, celix_array_list_t **references);

celix_status_t
serviceRegistry_ungetServiceReference(service_registry_pt registry, celix_bundle_t *bundle, service_reference_pt reference);

celix_status_t serviceRegistry_clearReferencesFor(service_registry_pt registry, celix_bundle_t *bundle);

size_t serviceRegistry_nrOfHooks(service_registry_pt registry);
@@ -275,7 +275,7 @@ celix_status_t bundleContext_ungetServiceReference(bundle_context_pt context, se
celix_status_t status = CELIX_SUCCESS;

if (context != NULL && reference != NULL && bundleContext_IsServiceReferenceValid(context, reference)) {
status = framework_ungetServiceReference(context->framework, context->bundle, reference);
serviceReference_release(reference, NULL);
} else {
status = CELIX_ILLEGAL_ARGUMENT;
}
@@ -1198,7 +1198,7 @@ static void celix_bundleContext_useServiceWithOptions_3_CloseServiceTracker(void
bool celix_bundleContext_useServiceWithOptions(
celix_bundle_context_t *ctx,
const celix_service_use_options_t *opts) {
if (opts == NULL) {
if (opts == NULL || opts->filter.serviceName == NULL) {
return false;
}

@@ -1253,7 +1253,7 @@ static void celix_bundleContext_useServicesWithOptions_2_UseServiceTracker(void
size_t celix_bundleContext_useServicesWithOptions(
celix_bundle_context_t *ctx,
const celix_service_use_options_t *opts) {
if (opts == NULL) {
if (opts == NULL || opts->filter.serviceName == NULL) {
return 0;
}

@@ -984,7 +984,7 @@ celix_status_t fw_getServiceReferences(framework_pt framework, array_list_pt *re
if (status == CELIX_SUCCESS) {
serviceNameObjectClass = properties_get(props, OSGI_FRAMEWORK_OBJECTCLASS);
if (!serviceReference_isAssignableTo(ref, bundle, serviceNameObjectClass)) {
//FIXME: unget service reference
serviceReference_release(ref, NULL);
arrayList_remove(*references, refIdx);
refIdx--;
}
@@ -997,10 +997,6 @@ celix_status_t fw_getServiceReferences(framework_pt framework, array_list_pt *re
return status;
}

celix_status_t framework_ungetServiceReference(framework_pt framework, bundle_pt bundle, service_reference_pt reference) {
return serviceRegistry_ungetServiceReference(framework->registry, bundle, reference);
}

celix_status_t fw_getBundleRegisteredServices(framework_pt framework, bundle_pt bundle, array_list_pt *services) {
return serviceRegistry_getRegisteredServices(framework->registry, bundle, services);
}
@@ -187,7 +187,6 @@ FRAMEWORK_EXPORT celix_status_t fw_registerServiceFactory(framework_pt framework
FRAMEWORK_EXPORT void fw_unregisterService(service_registration_pt registration);

FRAMEWORK_EXPORT celix_status_t fw_getServiceReferences(framework_pt framework, array_list_pt *references, bundle_pt bundle, const char* serviceName, const char* filter);
FRAMEWORK_EXPORT celix_status_t framework_ungetServiceReference(framework_pt framework, bundle_pt bundle, service_reference_pt reference);
FRAMEWORK_EXPORT celix_status_t fw_getBundleRegisteredServices(framework_pt framework, bundle_pt bundle, array_list_pt *services);
FRAMEWORK_EXPORT celix_status_t fw_getBundleServicesInUse(framework_pt framework, bundle_pt bundle, array_list_pt *services);

@@ -28,13 +28,16 @@
#define REGISTRY_CALLBACK_H_

#include "celix_errno.h"
#include "celix_types.h"
#include "service_reference.h"
#include "service_registration.h"
#include <stdbool.h>

typedef struct registry_callback_struct {
void *handle;
celix_status_t (*getUsingBundles)(void *handle, service_registration_pt reg, array_list_pt *bundles);
celix_status_t (*unregister)(void *handle, bundle_pt bundle, service_registration_pt reg);
bool (*tryRemoveServiceReference)(void *handle, service_reference_pt ref);
} registry_callback_t;

#endif /* REGISTRY_CALLBACK_H_ */
@@ -37,7 +37,7 @@
#include "service_registration_private.h"
#include "celix_build_assert.h"

static void serviceReference_doRelease(struct celix_ref *);
static bool serviceReference_doRelease(struct celix_ref *);
static void serviceReference_destroy(service_reference_pt);

celix_status_t serviceReference_create(registry_callback_t callback, bundle_pt referenceOwner, service_registration_pt registration, service_reference_pt *out) {
@@ -81,12 +81,20 @@ celix_status_t serviceReference_release(service_reference_pt ref, bool *out) {
return CELIX_SUCCESS;
}

static void serviceReference_doRelease(struct celix_ref *refCount) {
static bool serviceReference_doRelease(struct celix_ref *refCount) {
service_reference_pt ref = (service_reference_pt)refCount;
bool removed = true;
CELIX_BUILD_ASSERT(offsetof(struct serviceReference, refCount) == 0);
serviceReference_invalidateCache(ref);
serviceRegistration_release(ref->registration);
serviceReference_destroy(ref);
// now the reference is dying in the registry, to stop anyone from reviving it, we must remove it from the registry.
// suppose another thread revives the reference and release it immediately, we may end up with two (or more) callers of tryRemoveServiceReference.
// it's the registry's responsibility to guarantee that only one caller get the chance to actually perform cleanup
removed = ref->callback.tryRemoveServiceReference(ref->callback.handle, ref);
if(removed) {
serviceReference_invalidateCache(ref);
serviceRegistration_release(ref->registration);
serviceReference_destroy(ref);
}
return removed;
}

celix_status_t serviceReference_getUsageCount(service_reference_pt ref, size_t *count) {
@@ -25,7 +25,7 @@
#include "celix_constants.h"
#include "celix_build_assert.h"

static void serviceRegistration_destroy(struct celix_ref *);
static bool serviceRegistration_destroy(struct celix_ref *);
static celix_status_t serviceRegistration_initializeProperties(service_registration_pt registration, properties_pt properties);
static celix_status_t serviceRegistration_createInternal(registry_callback_t callback, bundle_pt bundle, const char* serviceName, unsigned long serviceId,
const void * serviceObject, properties_pt dictionary, enum celix_service_type svcType, service_registration_pt *registration);
@@ -77,7 +77,7 @@ void serviceRegistration_release(service_registration_pt registration) {
celix_ref_put(&registration->refCount, serviceRegistration_destroy);
}

static void serviceRegistration_destroy(struct celix_ref *refCount) {
static bool serviceRegistration_destroy(struct celix_ref *refCount) {
service_registration_pt registration = (service_registration_pt )refCount;
CELIX_BUILD_ASSERT(offsetof(service_registration_t, refCount) == 0);

@@ -88,6 +88,7 @@ static void serviceRegistration_destroy(struct celix_ref *refCount) {
properties_destroy(registration->properties);
celixThreadRwlock_destroy(&registration->lock);
free(registration);
return true;
}

static celix_status_t serviceRegistration_initializeProperties(service_registration_pt registration, properties_pt dictionary) {

0 comments on commit ef1202f

Please sign in to comment.