[GitHub] [celix] pnoltes commented on a change in pull request #399: Service tracker improvements

[GitBox <gi...@apache.org>]

pnoltes commented on a change in pull request #399:
URL: https://github.com/apache/celix/pull/399#discussion_r816211913



##########
File path: libs/framework/include/celix_bundle_context.h
##########
@@ -771,6 +771,12 @@ typedef struct celix_service_use_options {
      * and the bundle owning the service will also be provided to the callback.
      */
     void (*useWithOwner)(void *handle, void *svc, const celix_properties_t *props, const celix_bundle_t *svcOwner) OPTS_INIT;
+    /**
+     * @brief Call the provided callbacks from the caller thread directly if set, otherwise the callbacks will be called from the Celix event loop (most likely indirectly).
+     * Note that using blocking service in the Celix event loop is generally a bad idea, which should be avoided if possible.
+     */
+#define CELIX_SERVICE_USE_DIRECT          (1)
+    int flags;

Review comment:
       For C++ support the `OPTS_INIT` should be added so that all the options fields have a default initialisation.  

##########
File path: libs/framework/src/framework_private.h
##########
@@ -162,6 +162,13 @@ struct celix_framework {
         int eventQueueSize;
         int eventQueueFirstEntry;
         celix_array_list_t *dynamicEventQueue; //entry = celix_framework_event_t*. Used when the eventQueue is full
+        struct {

Review comment:
       nice 👍 stats addition. 

##########
File path: libs/framework/src/service_tracker.c
##########
@@ -226,10 +229,10 @@ celix_status_t serviceTracker_close(service_tracker_t* tracker) {
 
             if (tracked != NULL) {
                 int currentSize = nrOfTrackedEntries - 1;
-                serviceTracker_untrackTracked(tracker, tracked, currentSize);
+                serviceTracker_untrackTracked(tracker, tracked, currentSize, currentSize == 0);

Review comment:
       nice catch (currentSize == 0) to prevent unneeded callbacks. 

##########
File path: libs/framework/include/service_registration.h
##########
@@ -39,9 +39,6 @@ FRAMEWORK_EXPORT celix_status_t serviceRegistration_unregister(service_registrat
 FRAMEWORK_EXPORT celix_status_t
 serviceRegistration_getProperties(service_registration_t *registration, celix_properties_t **properties);
 
-FRAMEWORK_EXPORT celix_status_t
-serviceRegistration_setProperties(service_registration_t *registration, celix_properties_t *properties);

Review comment:
       Yeah I agree. 
   Ideally we do not update the public headers (even the ones without a celix_ prefix) backwards incompatible. But if it not used in Celix, probably also not used by users and it is problematic then we should make an exception.
   
   Side note: In the future I would like to make a 3.0 Celix release where all non celix_ prefix c headers are no longer installed and moved to the src dirs.   

##########
File path: libs/framework/src/service_registration.c
##########
@@ -135,71 +131,71 @@ bool serviceRegistration_isValid(service_registration_pt registration) {
 
 celix_status_t serviceRegistration_unregister(service_registration_pt registration) {
 	celix_status_t status = CELIX_SUCCESS;
-
-    bool notValidOrUnregistering;
-    celixThreadRwlock_readLock(&registration->lock);
-    notValidOrUnregistering = !serviceRegistration_isValid(registration) || registration->isUnregistering;
-    celixThreadRwlock_unlock(&registration->lock);
-
+    bool unregistering = false;
     registry_callback_t callback;
     callback.unregister = NULL;
-    bundle_pt bundle = NULL;
 
-	if (notValidOrUnregistering) {
-		status = CELIX_ILLEGAL_STATE;
-	} else {
-        celixThreadRwlock_writeLock(&registration->lock);
-        registration->isUnregistering = true;
-        bundle = registration->bundle;
-        callback = registration->callback;
-        celixThreadRwlock_unlock(&registration->lock);
+    // Without any further need of synchronization between callers, __ATOMIC_RELAXED should be sufficient to guarantee that only one caller has a chance to run.
+    // Strong form of compare-and-swap is used to avoid spurious failure.
+    if(!__atomic_compare_exchange_n(&registration->isUnregistering, &unregistering /* expected*/ , true /* desired */,
+                                    false /* weak */, __ATOMIC_RELAXED/*success memorder*/, __ATOMIC_RELAXED/*failure memorder*/)) {
+        status = CELIX_ILLEGAL_STATE;
+        goto immediate_return;

Review comment:
       is the goto needed?
   
   I think the lines 145, 146 and 147 can go into an else {} part. 
   
   I rather avoid usage of goto

##########
File path: libs/framework/src/bundle_context.c
##########
@@ -1181,78 +1182,70 @@ static void celix_bundleContext_useServiceWithOptions_2_UseServiceTracker(void *
     celix_bundle_context_use_service_data_t* d = data;
     assert(celix_framework_isCurrentThreadTheEventLoop(d->ctx->framework));
 
-    d->called = celix_serviceTracker_useHighestRankingService(d->svcTracker, d->opts->filter.serviceName, d->opts->callbackHandle, d->opts->use, d->opts->useWithProperties, d->opts->useWithOwner);
-}
-
-static void celix_bundleContext_useServiceWithOptions_3_CloseServiceTracker(void *data) {
-    celix_bundle_context_use_service_data_t* d = data;
-    assert(celix_framework_isCurrentThreadTheEventLoop(d->ctx->framework));
-
-    celix_service_tracker_t *tracker = d->svcTracker;
-    d->svcTracker = NULL;
-
-    celix_serviceTracker_destroy(tracker);
+    d->called = celix_serviceTracker_useHighestRankingService(d->svcTracker, d->opts->filter.serviceName, 0, d->opts->callbackHandle, d->opts->use, d->opts->useWithProperties, d->opts->useWithOwner);
 }
 
 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;
     }
 
     celix_bundle_context_use_service_data_t data = {0};
     data.ctx = ctx;
     data.opts = opts;
+    bool called = false;
 
     if (celix_framework_isCurrentThreadTheEventLoop(ctx->framework)) {
         // Ignore timeout: blocking the event loop prevents any progress to be made
         celix_bundleContext_useServiceWithOptions_1_CreateServiceTracker(&data);
         celix_bundleContext_useServiceWithOptions_2_UseServiceTracker(&data);
-        celix_bundleContext_useServiceWithOptions_3_CloseServiceTracker(&data);
+        celix_serviceTracker_destroy(data.svcTracker);
         return data.called;
     }
 
     long eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "create service tracker for celix_bundleContext_useServiceWithOptions", &data, celix_bundleContext_useServiceWithOptions_1_CreateServiceTracker, NULL, NULL);
     celix_framework_waitForGenericEvent(ctx->framework, eventId);
 
-    celix_framework_waitForEmptyEventQueue(ctx->framework); //ensure that a useService wait if a listener hooks concept, which triggers an async service registration
 
-    struct timespec startTime = celix_gettime(CLOCK_MONOTONIC);
-    bool useServiceIsDone = false;
-    bool called = false;
-    do {
-        eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "use service tracker for celix_bundleContext_useServiceWithOptions", &data, celix_bundleContext_useServiceWithOptions_2_UseServiceTracker, NULL, NULL);
-        celix_framework_waitForGenericEvent(ctx->framework, eventId);
+    if(opts->flags & CELIX_SERVICE_USE_DIRECT) {
+        celix_framework_waitUntilNoPendingRegistration(ctx->framework); // TL;DR make "service on demand" pattern work. Try comment it out and run CelixBundleContextServicesTests.UseServiceOnDemandDirectlyWithAsyncRegisterTest

Review comment:
       @PengZheng WDYT?

##########
File path: libs/framework/gtest/src/bundle_context_services_test.cpp
##########
@@ -61,6 +61,189 @@ class CelixBundleContextServicesTests : public ::testing::Test {
     CelixBundleContextServicesTests(const CelixBundleContextServicesTests&) = delete;
     CelixBundleContextServicesTests& operator=(CelixBundleContextServicesTests&&) = delete;
     CelixBundleContextServicesTests& operator=(const CelixBundleContextServicesTests&) = delete;
+
+    void registerAndUseServiceWithCorrectVersion(bool direct) {
+        struct calc {
+            int (*calc)(int);
+        };
+
+        const char *calcName = "calc";
+        struct calc svc;
+        svc.calc = [](int n) -> int {
+            return n * 42;
+        };
+
+        celix_service_use_options_t use_opts{};
+        use_opts.filter.serviceName = "calc";
+        use_opts.filter.versionRange = "[1,2)";
+        if(direct) {
+            use_opts.flags = CELIX_SERVICE_USE_DIRECT;
+        }
+
+        celix_service_registration_options_t reg_opts{};
+        reg_opts.serviceName = calcName;
+        reg_opts.serviceVersion = "1.5";
+        reg_opts.svc = &svc;
+
+        bool called = celix_bundleContext_useServiceWithOptions(ctx, &use_opts);
+        ASSERT_TRUE(!called); //service not avail.
+
+        std::future<bool> result{std::async([&]{
+            use_opts.waitTimeoutInSeconds = 5.0;
+            printf("Trying to call calc with timeout of %f\n", use_opts.waitTimeoutInSeconds);
+            bool calledAsync = celix_bundleContext_useServiceWithOptions(ctx, &use_opts);
+            printf("returned from use service with timeout. calc called %s.\n", calledAsync ? "true" : "false");
+            return calledAsync;
+        })};
+        long svcId = celix_bundleContext_registerServiceWithOptions(ctx, &reg_opts);
+        ASSERT_TRUE(svcId >= 0);
+
+        ASSERT_TRUE(result.get()); //should return true after waiting for the registered service.
+
+        celix_bundleContext_unregisterService(ctx, svcId);
+    }

Review comment:
       nitpick: I prefer a empty new line between functions/methods. 

##########
File path: libs/framework/include/celix_bundle_context.h
##########
@@ -771,6 +771,12 @@ typedef struct celix_service_use_options {
      * and the bundle owning the service will also be provided to the callback.
      */
     void (*useWithOwner)(void *handle, void *svc, const celix_properties_t *props, const celix_bundle_t *svcOwner) OPTS_INIT;
+    /**
+     * @brief Call the provided callbacks from the caller thread directly if set, otherwise the callbacks will be called from the Celix event loop (most likely indirectly).
+     * Note that using blocking service in the Celix event loop is generally a bad idea, which should be avoided if possible.
+     */
+#define CELIX_SERVICE_USE_DIRECT          (1)

Review comment:
       I agree that `CELIX_SERVICE_USE_DIRECT` ideally should be the default (mail in dev mailing list)
   
   Although it is good that "service on demand" can be triggered with useService(s), this is a more corner use case and especially useful for testing purpose. 

##########
File path: libs/framework/src/service_registry.c
##########
@@ -277,13 +277,12 @@ celix_status_t serviceRegistry_unregisterService(service_registry_pt registry, b
         service_reference_pt ref = refsMap != NULL ?
                                    hashMap_get(refsMap, (void*)registration->serviceId) : NULL;
         if (ref != NULL) {
-            serviceReference_invalidate(ref);
+            serviceReference_invalidateCache(ref);
         }
     }
     hashMapIterator_destroy(iter);
+    serviceRegistration_invalidate(registration);
 	celixThreadRwlock_unlock(&registry->lock);

Review comment:
       Just to be sure. There **was** a window for a race condition, right? 
   
   Because now that the serviceRegistration_invalidate has moved to before the unlock, this should not happen anymore. 

##########
File path: libs/framework/src/bundle_context.c
##########
@@ -1181,78 +1182,70 @@ static void celix_bundleContext_useServiceWithOptions_2_UseServiceTracker(void *
     celix_bundle_context_use_service_data_t* d = data;
     assert(celix_framework_isCurrentThreadTheEventLoop(d->ctx->framework));
 
-    d->called = celix_serviceTracker_useHighestRankingService(d->svcTracker, d->opts->filter.serviceName, d->opts->callbackHandle, d->opts->use, d->opts->useWithProperties, d->opts->useWithOwner);
-}
-
-static void celix_bundleContext_useServiceWithOptions_3_CloseServiceTracker(void *data) {
-    celix_bundle_context_use_service_data_t* d = data;
-    assert(celix_framework_isCurrentThreadTheEventLoop(d->ctx->framework));
-
-    celix_service_tracker_t *tracker = d->svcTracker;
-    d->svcTracker = NULL;
-
-    celix_serviceTracker_destroy(tracker);
+    d->called = celix_serviceTracker_useHighestRankingService(d->svcTracker, d->opts->filter.serviceName, 0, d->opts->callbackHandle, d->opts->use, d->opts->useWithProperties, d->opts->useWithOwner);
 }
 
 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;
     }
 
     celix_bundle_context_use_service_data_t data = {0};
     data.ctx = ctx;
     data.opts = opts;
+    bool called = false;
 
     if (celix_framework_isCurrentThreadTheEventLoop(ctx->framework)) {
         // Ignore timeout: blocking the event loop prevents any progress to be made
         celix_bundleContext_useServiceWithOptions_1_CreateServiceTracker(&data);
         celix_bundleContext_useServiceWithOptions_2_UseServiceTracker(&data);
-        celix_bundleContext_useServiceWithOptions_3_CloseServiceTracker(&data);
+        celix_serviceTracker_destroy(data.svcTracker);
         return data.called;
     }
 
     long eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "create service tracker for celix_bundleContext_useServiceWithOptions", &data, celix_bundleContext_useServiceWithOptions_1_CreateServiceTracker, NULL, NULL);
     celix_framework_waitForGenericEvent(ctx->framework, eventId);
 
-    celix_framework_waitForEmptyEventQueue(ctx->framework); //ensure that a useService wait if a listener hooks concept, which triggers an async service registration
 
-    struct timespec startTime = celix_gettime(CLOCK_MONOTONIC);
-    bool useServiceIsDone = false;
-    bool called = false;
-    do {
-        eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "use service tracker for celix_bundleContext_useServiceWithOptions", &data, celix_bundleContext_useServiceWithOptions_2_UseServiceTracker, NULL, NULL);
-        celix_framework_waitForGenericEvent(ctx->framework, eventId);
+    if(opts->flags & CELIX_SERVICE_USE_DIRECT) {
+        celix_framework_waitUntilNoPendingRegistration(ctx->framework); // TL;DR make "service on demand" pattern work. Try comment it out and run CelixBundleContextServicesTests.UseServiceOnDemandDirectlyWithAsyncRegisterTest

Review comment:
       IMO this can be removed. As long as we document that `CELIX_SERVICE_USE_DIRECT` will not work with the "service on demand" pattern. 
   
   This should also make it possible to work directly on service registry to prevent even more overhead (future, if we want to and this will be a significant refactoring).  




-- 
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-unsubscribe@celix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org