PengZheng commented on code in PR #734:
URL: https://github.com/apache/celix/pull/734#discussion_r1510524927
##########
libs/framework/src/bundle_context.c:
##########
@@ -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_RELAXED) > 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(1);
Review Comment:
With high resolution timer enabled for linux kernel, this will lead to high
CPU usage. Is 1ms/10ms acceptable? I think they may be better.
##########
libs/framework/src/bundle_context.c:
##########
@@ -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[32]; //20 is max long length
+ (void)snprintf(filter, sizeof(filter), "(%s=%li)",
CELIX_FRAMEWORK_SERVICE_ID, serviceId);
Review Comment:
A long can take value `INT64_MIN`, which need 20 bytes(including `-`). 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.
##########
libs/framework/src/bundle_context.c:
##########
@@ -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_RELAXED) > 0) {
Review Comment:
This needs to be `__ATOMIC_ACQUIRE`, otherwise the destroying of tracker may
seems like happen before this from the service user's point of view.
##########
libs/framework/src/bundle_context.c:
##########
@@ -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);
+ }
+ 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_RELAXED);
Review Comment:
This has to be `__ATOMIC_RELEASE`, otherwise use of service may seem like
happen after this from the service stopper's point of view.
##########
libs/framework/src/bundle_context.c:
##########
@@ -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);
Review Comment:
This use of `__ATOMIC_RELAXED` is indeed correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
