Feature/87 create service tracker inside use tracked service call #734
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #734 +/- ##
==========================================
+ Coverage 89.57% 89.58% +0.01%
==========================================
Files 216 216
Lines 25395 25413 +18
==========================================
+ Hits 22747 22766 +19
+ Misses 2648 2647 -1 ☔ View full report in Codecov by Sentry. |
libs/framework/src/bundle_context.c
Outdated
| start = celix_gettime(CLOCK_MONOTONIC); | ||
| logCount++; | ||
| } | ||
| usleep(1); |
There was a problem hiding this 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
Outdated
| // 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) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks. I assumed ordering was not relevant, because a count increased only happened in the shared lock.
But indeed this check and thus the destroy of the tracking needs to be ordered after the use count decrease.
libs/framework/src/bundle_context.c
Outdated
| } | ||
|
|
||
| (void)__atomic_fetch_sub(&trkEntry->useCount, 1, __ATOMIC_RELAXED); |
There was a problem hiding this 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.
| 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); |
There was a problem hiding this comment.
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).
| 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); |
There was a problem hiding this comment.
This use of __ATOMIC_RELAXED is indeed correct.
1. Use acquire-release pair to synchronize the service user with the service stopper. 2. Reduce CPU usage when waiting for service user. 3. Fix gcc -Wstringop-truncation warning.
pnoltes
deleted the
feature/87-create-service-tracker-inside-useTrackedService-call
branch
This PR adds support for creating a service tracking in a
celix_bundleContext_useTrackedService(s)*calls.This is done by calling the use callbacks outside of the (read) lock. The prevent that the service tracker can be removed during a use callback, an atomic use count is introduced.