From ee26a6d4bacddc0cd29735ec10f3ea8cf7417011 Mon Sep 17 00:00:00 2001 From: PengZheng Date: Tue, 1 Mar 2022 16:28:32 +0800 Subject: [PATCH] Using service directly does not support SOD by default. To request such support, the caller has to explicitly set CELIX_SERVICE_USE_SOD flags. Other code review issues are also addressed. --- .../gtest/src/bundle_context_services_test.cpp | 7 +++++-- libs/framework/include/celix_bundle_context.h | 9 +++++++-- libs/framework/src/bundle_context.c | 10 ++++++++-- libs/framework/src/service_registration.c | 11 +++++------ 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/libs/framework/gtest/src/bundle_context_services_test.cpp b/libs/framework/gtest/src/bundle_context_services_test.cpp index 097541a6e..352fc09c0 100644 --- a/libs/framework/gtest/src/bundle_context_services_test.cpp +++ b/libs/framework/gtest/src/bundle_context_services_test.cpp @@ -102,6 +102,7 @@ class CelixBundleContextServicesTests : public ::testing::Test { celix_bundleContext_unregisterService(ctx, svcId); } + void registerAndUseServiceWithIncorrectVersion(bool direct) { struct calc { int (*calc)(int); @@ -141,6 +142,7 @@ class CelixBundleContextServicesTests : public ::testing::Test { celix_bundleContext_unregisterService(ctx, svcId); } + void registerAndUseServiceWithTimeout(bool direct) { const int NR_ITERATIONS = 5; //NOTE this test is sensitive for triggering race condition in the celix framework, therefore is used a few times. for (int i = 0; i < NR_ITERATIONS; ++i) { @@ -192,6 +194,7 @@ class CelixBundleContextServicesTests : public ::testing::Test { EXPECT_FALSE(result2.get()); //note service is away, so even with a wait the service is not found. } } + void registerAsyncAndUseServiceWithTimeout(bool direct) { const int NR_ITERATIONS = 5; //NOTE this test is sensitive for triggering race condition in the celix framework, therefore is used a few times. for (int i = 0; i < NR_ITERATIONS; ++i) { @@ -1442,7 +1445,7 @@ TEST_F(CelixBundleContextServicesTests, UseServiceOnDemandDirectlyWithAsyncRegis }, nullptr); celix_service_use_options_t opts{}; opts.filter.serviceName = "test"; - opts.flags = CELIX_SERVICE_USE_DIRECT; + opts.flags = CELIX_SERVICE_USE_DIRECT | CELIX_SERVICE_USE_SOD; called = celix_bundleContext_useServiceWithOptions(ctx, &opts); EXPECT_TRUE(called); //service created on demand. @@ -1480,7 +1483,7 @@ TEST_F(CelixBundleContextServicesTests, UseServicesOnDemandDirectlyWithAsyncRegi celix_service_use_options_t opts{}; opts.filter.serviceName = "test"; - opts.flags = CELIX_SERVICE_USE_DIRECT; + opts.flags = CELIX_SERVICE_USE_DIRECT | CELIX_SERVICE_USE_SOD; size_t count = celix_bundleContext_useServicesWithOptions(ctx, &opts); EXPECT_EQ(2, count); diff --git a/libs/framework/include/celix_bundle_context.h b/libs/framework/include/celix_bundle_context.h index 9651c3aa5..214c5d052 100644 --- a/libs/framework/include/celix_bundle_context.h +++ b/libs/framework/include/celix_bundle_context.h @@ -775,8 +775,13 @@ typedef struct celix_service_use_options { * @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; +#define CELIX_SERVICE_USE_DIRECT (1) + /** + * @brief Whether "service on demand" pattern is supported when CELIX_SERVICE_USE_DIRECT is set. + * Note that it has no effect in indirect mode, in which case "service on demand" is supported. + */ +#define CELIX_SERVICE_USE_SOD (2) + int flags OPTS_INIT; } celix_service_use_options_t; /** diff --git a/libs/framework/src/bundle_context.c b/libs/framework/src/bundle_context.c index 40d3e700b..e1c0eeec4 100644 --- a/libs/framework/src/bundle_context.c +++ b/libs/framework/src/bundle_context.c @@ -1210,7 +1210,10 @@ bool celix_bundleContext_useServiceWithOptions( 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 + if(opts->flags & CELIX_SERVICE_USE_SOD) { + // TL;DR make "service on demand" pattern work. Try comment it out and run CelixBundleContextServicesTests.UseServiceOnDemandDirectlyWithAsyncRegisterTest + celix_framework_waitUntilNoPendingRegistration(ctx->framework); + } called = celix_serviceTracker_useHighestRankingService(data.svcTracker, NULL, opts->waitTimeoutInSeconds, opts->callbackHandle, opts->use, opts->useWithProperties, opts->useWithOwner); } else { struct timespec startTime = celix_gettime(CLOCK_MONOTONIC); @@ -1264,7 +1267,10 @@ size_t celix_bundleContext_useServicesWithOptions( 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.UseServicesOnDemandDirectlyWithAsyncRegisterTest + if(opts->flags & CELIX_SERVICE_USE_SOD) { + // TL;DR make "service on demand" pattern work. Try comment it out and run CelixBundleContextServicesTests.UseServicesOnDemandDirectlyWithAsyncRegisterTest + celix_framework_waitUntilNoPendingRegistration(ctx->framework); + } celix_bundleContext_useServicesWithOptions_2_UseServiceTracker(&data); } else { eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "use service tracker for celix_bundleContext_useServicesWithOptions", &data, celix_bundleContext_useServicesWithOptions_2_UseServiceTracker, NULL, NULL); diff --git a/libs/framework/src/service_registration.c b/libs/framework/src/service_registration.c index b767844d0..f4832c51b 100644 --- a/libs/framework/src/service_registration.c +++ b/libs/framework/src/service_registration.c @@ -140,14 +140,13 @@ celix_status_t serviceRegistration_unregister(service_registration_pt registrati if(!__atomic_compare_exchange_n(®istration->isUnregistering, &unregistering /* expected*/ , true /* desired */, false /* weak */, __ATOMIC_RELAXED/*success memorder*/, __ATOMIC_RELAXED/*failure memorder*/)) { status = CELIX_ILLEGAL_STATE; - goto immediate_return; + } else { + callback = registration->callback; + if (callback.unregister != NULL) { + status = callback.unregister(callback.handle, registration->bundle, registration); + } } - callback = registration->callback; - if (callback.unregister != NULL) { - status = callback.unregister(callback.handle, registration->bundle, registration); - } -immediate_return: framework_logIfError(celix_frameworkLogger_globalLogger(), status, NULL, "Cannot unregister service registration"); return status; }