Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/87 create service tracker inside use tracked service call #734

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Feature/87 create service tracker inside use tracked service call #734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbadfeb
gh-87: Add test to call useTrackedService on the event thread.
pnoltes Mar 2, 2024
e665364
gh-87: Support creating service tracker inside a useTrackedService call
pnoltes Mar 3, 2024
3c3227a
gh-87: Address review issues in gh-734.
PengZheng Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions libs/framework/gtest/src/CelixBundleContextServicesTestSuite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2009,3 +2009,62 @@ TEST_F(CelixBundleContextServicesTestSuite, UseTrackedServiceDuringTrackerCreati
serviceFound = celix_bundleContext_useTrackedService(ctx, trkId, nullptr, nullptr);
EXPECT_TRUE(serviceFound);
}

TEST_F(CelixBundleContextServicesTestSuite, UseTrackedServiceOnTheCelixEventThread) {
//Given a registered service with a service registration guard
long svcId = celix_bundleContext_registerService(ctx, (void*)0x42, "test", nullptr);
celix_auto(celix_service_registration_guard_t) svcGuard = celix_serviceRegistrationGuard_init(ctx, svcId);

//And a service tracker for the "test" service with a service tracker guard
long trkId = celix_bundleContext_trackServices(ctx, "test");
celix_auto(celix_tracker_guard_t) trkGuard = celix_trackerGuard_init(ctx, trkId);

//When all events are processed
celix_bundleContext_waitForEvents(ctx);

//Then I can use the tracked service on the Celix event thread
struct callback_data {
celix_bundle_context_t* ctx;
long trkId;
};
callback_data cbData{ctx, trkId};
auto eventCallback = [](void *data) {
auto d = static_cast<callback_data*>(data);
bool called = celix_bundleContext_useTrackedService(d->ctx, d->trkId, nullptr, nullptr);
EXPECT_TRUE(called);
};

long eventId = celix_framework_fireGenericEvent(
fw,
-1,
celix_bundle_getId(celix_framework_getFrameworkBundle(fw)),
"use tracked service",
(void*)&cbData,
eventCallback,
nullptr,
nullptr);
celix_framework_waitForGenericEvent(fw, eventId);
}

TEST_F(CelixBundleContextServicesTestSuite, CreateServiceTrackedOnUseServiceTrackerCall) {
//Given a registered service with a service registration guard
long svcId = celix_bundleContext_registerService(ctx, (void*)0x42, "test", nullptr);
celix_auto(celix_service_registration_guard_t) svcGuard = celix_serviceRegistrationGuard_init(ctx, svcId);

//And a service tracker for the "test" service with a service tracker guard
long trkId = celix_bundleContext_trackServices(ctx, "test");
celix_auto(celix_tracker_guard_t) trkGuard = celix_trackerGuard_init(ctx, trkId);

//When all events are processed
celix_bundleContext_waitForEvents(ctx);

//Then I can create and destroy an additional service tracker on the callback of the useTrackedService function
auto useCallback = [](void *data, void* /*svc*/) {
auto c = static_cast<celix_bundle_context_t *>(data);
long additionalTrkId = celix_bundleContext_trackServices(c, "foo");
EXPECT_GT(additionalTrkId, 0);
celix_bundleContext_stopTracker(c, additionalTrkId);
};
bool called = celix_bundleContext_useTrackedService(ctx, trkId, ctx, useCallback);
EXPECT_TRUE(called);
}
111 changes: 73 additions & 38 deletions libs/framework/src/bundle_context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
#include "celix_array_list.h"
#include "celix_convert_utils.h"

#define TRACKER_WARN_THRESHOLD_SEC 5

static celix_status_t bundleContext_bundleChanged(void* listenerSvc, bundle_event_t* event);
static void bundleContext_cleanupBundleTrackers(bundle_context_t *ct);
static void bundleContext_cleanupServiceTrackers(bundle_context_t *ctx);
Expand Down Expand Up @@ -839,7 +841,28 @@ static void celix_bundleContext_removeServiceTrackerTracker(void *data) {
free(tracker);
}

static void celix_bundleContext_stopTrackerInternal(bundle_context_t *ctx, long trackerId, bool async, void *doneData, void (*doneCallback)(void* doneData)) {
static void celix_bundleContext_waitForUnusedServiceTracker(celix_bundle_context_t* ctx,
celix_bundle_context_service_tracker_entry_t* trkEntry) {
// busy wait till the tracker is not used anymore
// note that the use count cannot be increased anymore, because the tracker is removed from the map
struct timespec start = celix_gettime(CLOCK_MONOTONIC);
int logCount = 0;
while (__atomic_load_n(&trkEntry->useCount, __ATOMIC_ACQUIRE) > 0) {
if (celix_elapsedtime(CLOCK_MONOTONIC, start) > TRACKER_WARN_THRESHOLD_SEC) {
fw_log(ctx->framework->logger,
CELIX_LOG_LEVEL_WARNING,
"Service tracker with trk id %li is still in use after %i seconds. "
"This might indicate a programming error.",
trkEntry->trackerId,
logCount * TRACKER_WARN_THRESHOLD_SEC);
start = celix_gettime(CLOCK_MONOTONIC);
logCount++;
}
usleep(1000);
}
}

static void celix_bundleContext_stopTrackerInternal(celix_bundle_context_t* ctx, long trackerId, bool async, void *doneData, void (*doneCallback)(void* doneData)) {
if (ctx == NULL || trackerId <= 0) {
return;
}
Expand Down Expand Up @@ -888,6 +911,7 @@ static void celix_bundleContext_stopTrackerInternal(bundle_context_t *ctx, long
fw_removeBundleListener(ctx->framework, ctx->bundle, &bundleTracker->listener);
free(bundleTracker);
} else if (serviceTracker != NULL) {
celix_bundleContext_waitForUnusedServiceTracker(ctx, serviceTracker);
celix_serviceTracker_destroy(serviceTracker->tracker);
if (serviceTracker->isFreeFilterNeeded) {
free((char*)serviceTracker->opts.filter.serviceName);
Expand Down Expand Up @@ -1058,11 +1082,10 @@ bool celix_bundleContext_useServiceWithId(
const char *serviceName,
void *callbackHandle,
void (*use)(void *handle, void *svc)) {
celix_service_use_options_t opts = CELIX_EMPTY_SERVICE_USE_OPTIONS;

char filter[64];
snprintf(filter, 64, "(%s=%li)", CELIX_FRAMEWORK_SERVICE_ID, serviceId);
char filter[1+/*(*/sizeof(CELIX_FRAMEWORK_SERVICE_ID)-1+1/*=*/+20/*INT64_MIN*/+1/*)*/+1/*'\0'*/];
(void)snprintf(filter, sizeof(filter), "(%s=%li)", CELIX_FRAMEWORK_SERVICE_ID, serviceId);
Copy link
Contributor

@PengZheng PengZheng Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A long can take value INT64_MIN, which need 20 bytes(including the leading -). The total length of the buffer needs to be 1 (() + 10 (CELIX_FRAMEWORK_SERVICE_ID) + 1 (=) + 20 (%li) + 1 ()) + 1 ('\0') = 34 bytes. This modification will lead to gcc warning complaining string truncation (-Wstringop-truncation).


celix_service_use_options_t opts = CELIX_EMPTY_SERVICE_USE_OPTIONS;
opts.filter.serviceName = serviceName;
opts.filter.filter = filter;
opts.callbackHandle = callbackHandle;
Expand Down Expand Up @@ -1379,9 +1402,10 @@ size_t celix_bundleContext_useTrackedServices(celix_bundle_context_t* ctx,
}

/**
* @brief Find a service tracker with the given tracker id. ctx->mutex must be locked.
* @brief Find a service tracker with the given tracker id. ctx->lock must be taken.
*/
static celix_service_tracker_t* celix_bundleContext_findServiceTracker(celix_bundle_context_t* ctx, long trackerId) {
static celix_bundle_context_service_tracker_entry_t* celix_bundleContext_findServiceTracker(celix_bundle_context_t* ctx,
long trackerId) {
if (trackerId < 0) {
return NULL; // silent ignore
}
Expand All @@ -1396,40 +1420,46 @@ static celix_service_tracker_t* celix_bundleContext_findServiceTracker(celix_bun
}

if (!entry->tracker && entry->createEventId >= 0) { // tracker not yet created (async creation)
if (celix_framework_isCurrentThreadTheEventLoop(ctx->framework)) {
fw_log(
ctx->framework->logger,
CELIX_LOG_LEVEL_DEBUG,
"Tracker with id %li is not yet created.",
entry->trackerId);
}
fw_log(
ctx->framework->logger, CELIX_LOG_LEVEL_DEBUG, "Tracker with id %li is not yet created.", entry->trackerId);
return NULL;
}
return entry->tracker;
return entry;
}

static size_t celix_bundleContext_useTrackedServiceWithOptionsInternal(celix_bundle_context_t* ctx,
long trackerId,
const celix_tracked_service_use_options_t* opts,
bool singleUse) {
celix_auto(celix_rwlock_rlock_guard_t) lck = celixRwlockRlockGuard_init(&ctx->lock);
celix_service_tracker_t* trk = celix_bundleContext_findServiceTracker(ctx, trackerId);
if (!trk) {
celixThreadRwlock_readLock(&ctx->lock);
celix_bundle_context_service_tracker_entry_t* trkEntry = celix_bundleContext_findServiceTracker(ctx, trackerId);
if (trkEntry) {
// note use count is only increased inside a read (shared) lock and ensures that the trkEntry is not freed and
// the trkEntry->tracker is not destroyed until the use count drops back to 0.
(void)__atomic_fetch_add(&trkEntry->useCount, 1, __ATOMIC_RELAXED);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use of __ATOMIC_RELAXED is indeed correct.

}
celixThreadRwlock_unlock(&ctx->lock);

if (!trkEntry) {
return 0;
}

size_t callCount;
if (singleUse) {
bool called = celix_serviceTracker_useHighestRankingService(trk,
NULL,
0,
opts->callbackHandle,
opts->use,
opts->useWithProperties,
opts->useWithOwner);
return called ? 1 : 0;
callCount = celix_serviceTracker_useHighestRankingService(trkEntry->tracker,
NULL,
0,
opts->callbackHandle,
opts->use,
opts->useWithProperties,
opts->useWithOwner);
} else {
return celix_serviceTracker_useServices(
trk, NULL, opts->callbackHandle, opts->use, opts->useWithProperties, opts->useWithOwner);
callCount = celix_serviceTracker_useServices(
trkEntry->tracker, NULL, opts->callbackHandle, opts->use, opts->useWithProperties, opts->useWithOwner);
}

(void)__atomic_fetch_sub(&trkEntry->useCount, 1, __ATOMIC_RELEASE);
return callCount;
}

bool celix_bundleContext_useTrackedServiceWithOptions(celix_bundle_context_t* ctx,
Expand All @@ -1444,7 +1474,11 @@ size_t celix_bundleContext_useTrackedServicesWithOptions(celix_bundle_context_t*
return celix_bundleContext_useTrackedServiceWithOptionsInternal(ctx, trackerId, opts, false);
}

void celix_bundleContext_getTrackerInfo(celix_bundle_context_t *ctx, long trackerId, size_t *trackedServiceCount, const char **trackedServiceName, const char **trackedServiceFilter) {
static void celix_bundleContext_getTrackerInfo(celix_bundle_context_t* ctx,
long trackerId,
size_t* trackedServiceCount,
const char** trackedServiceName,
const char** trackedServiceFilter) {
if (trackedServiceCount) {
*trackedServiceCount = 0;
}
Expand All @@ -1456,19 +1490,19 @@ void celix_bundleContext_getTrackerInfo(celix_bundle_context_t *ctx, long tracke
}

celix_auto(celix_rwlock_rlock_guard_t) lck = celixRwlockRlockGuard_init(&ctx->lock);
celix_service_tracker_t* trk = celix_bundleContext_findServiceTracker(ctx, trackerId);
if (!trk) {
celix_bundle_context_service_tracker_entry_t* trkEntry = celix_bundleContext_findServiceTracker(ctx, trackerId);
if (!trkEntry) {
return;
}

if (trackedServiceCount) {
*trackedServiceCount = celix_serviceTracker_getTrackedServiceCount(trk);
*trackedServiceCount = celix_serviceTracker_getTrackedServiceCount(trkEntry->tracker);
}
if (trackedServiceName) {
*trackedServiceName = celix_serviceTracker_getTrackedServiceName(trk);
*trackedServiceName = celix_serviceTracker_getTrackedServiceName(trkEntry->tracker);
}
if (trackedServiceFilter) {
*trackedServiceFilter = celix_serviceTracker_getTrackedServiceFilter(trk);
*trackedServiceFilter = celix_serviceTracker_getTrackedServiceFilter(trkEntry->tracker);
}
}

Expand All @@ -1492,7 +1526,9 @@ const char* celix_bundleContext_getTrackedServiceFilter(celix_bundle_context_t*

bool celix_bundleContext_isValidTrackerId(celix_bundle_context_t* ctx, long trackerId) {
celix_auto(celix_rwlock_rlock_guard_t) lck = celixRwlockRlockGuard_init(&ctx->lock);
return celix_longHashMap_hasKey(ctx->serviceTrackers, trackerId);
return celix_longHashMap_hasKey(ctx->serviceTrackers, trackerId) ||
celix_longHashMap_hasKey(ctx->bundleTrackers, trackerId) ||
celix_longHashMap_hasKey(ctx->metaTrackers, trackerId);
}

long celix_bundleContext_findService(celix_bundle_context_t *ctx, const char *serviceName) {
Expand Down Expand Up @@ -1537,12 +1573,11 @@ celix_array_list_t* celix_bundleContext_findServicesWithOptions(celix_bundle_con
static celix_status_t bundleContext_callServicedTrackerTrackerCallback(void *handle, celix_array_list_t *listeners, bool add) {
celix_bundle_context_service_tracker_tracker_entry_t *entry = handle;
if (entry != NULL) {
size_t size = celix_arrayList_size(listeners);
for (unsigned int i = 0; i < size; ++i) {
int size = celix_arrayList_size(listeners);
for (int i = 0; i < size; ++i) {
listener_hook_info_pt info = celix_arrayList_get(listeners, i);
celix_bundle_t *bnd = NULL;
bundleContext_getBundle(info->context, &bnd);

celix_autoptr(celix_filter_t) filter = celix_filter_create(info->filter);
celix_service_tracker_info_t trkInfo;
memset(&trkInfo, 0, sizeof(trkInfo));
Expand Down
3 changes: 3 additions & 0 deletions libs/framework/src/bundle_context_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ typedef struct celix_bundle_context_service_tracker_entry {
// used for sync
long createEventId;
bool cancelled; // if tracker is stopped before created async

unsigned int useCount; // atomic, used to keep track of the number of times the tracker is used for a
// useTrackedService or useService call
} celix_bundle_context_service_tracker_entry_t;

typedef struct celix_bundle_context_service_tracker_tracker_entry {
Expand Down
Loading