You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@celix.apache.org by pn...@apache.org on 2020/05/05 12:25:18 UTC
[celix] branch feature/threadsafe_svc_id created (now 0dfa781)
This is an automated email from the ASF dual-hosted git repository.
pnoltes pushed a change to branch feature/threadsafe_svc_id
in repository https://gitbox.apache.org/repos/asf/celix.git.
at 0dfa781 Fixes an potential race conditation issues with the creation of service ids.
This branch includes the following new commits:
new 0dfa781 Fixes an potential race conditation issues with the creation of service ids.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[celix] 01/01: Fixes an potential race conditation issues with the
creation of service ids.
Posted by pn...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pnoltes pushed a commit to branch feature/threadsafe_svc_id
in repository https://gitbox.apache.org/repos/asf/celix.git
commit 0dfa781f5a2b8915e1cc616519784ad4f432250a
Author: Pepijn Noltes <pe...@gmail.com>
AuthorDate: Tue May 5 14:24:57 2020 +0200
Fixes an potential race conditation issues with the creation of service ids.
Also fixes some wrong "check if svcId/trackerId is valid" statements.
---
.../gtest/src/bundle_context_services_test.cpp | 24 ++---
libs/framework/include/service_registry.h | 6 ++
.../private/test/service_registry_test.cpp | 111 +--------------------
libs/framework/src/service_registry.c | 15 ++-
libs/framework/src/service_registry_private.h | 2 +-
libs/framework/src/service_tracker.c | 2 +-
6 files changed, 31 insertions(+), 129 deletions(-)
diff --git a/libs/framework/gtest/src/bundle_context_services_test.cpp b/libs/framework/gtest/src/bundle_context_services_test.cpp
index 17e0d89..e544d2f 100644
--- a/libs/framework/gtest/src/bundle_context_services_test.cpp
+++ b/libs/framework/gtest/src/bundle_context_services_test.cpp
@@ -392,15 +392,15 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerTest) {
};
long trackerId = celix_bundleContext_trackServices(ctx, "calc", &count, add, remove);
- ASSERT_TRUE(trackerId > 0);
+ ASSERT_TRUE(trackerId >= 0);
ASSERT_EQ(0, count);
long svcId1 = celix_bundleContext_registerService(ctx, (void*)0x100, "calc", nullptr);
- ASSERT_TRUE(svcId1 > 0);
+ ASSERT_TRUE(svcId1 >= 0);
ASSERT_EQ(1, count);
long svcId2 = celix_bundleContext_registerService(ctx, (void*)0x200, "calc", nullptr);
- ASSERT_TRUE(svcId2 > 0);
+ ASSERT_TRUE(svcId2 >= 0);
ASSERT_EQ(2, count);
celix_bundleContext_unregisterService(ctx, svcId1);
@@ -453,15 +453,15 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerTestWithAlreadyRegistered
long trackerId = celix_bundleContext_trackServices(ctx, "calc", &count, add, remove);
- ASSERT_TRUE(trackerId > 0);
+ ASSERT_TRUE(trackerId >= 0);
ASSERT_EQ(2, count);
long svcId3 = celix_bundleContext_registerService(ctx, (void*)0x100, "calc", nullptr);
- ASSERT_TRUE(svcId1 > 0);
+ ASSERT_TRUE(svcId1 >= 0);
ASSERT_EQ(3, count);
long svcId4 = celix_bundleContext_registerService(ctx, (void*)0x200, "calc", nullptr);
- ASSERT_TRUE(svcId2 > 0);
+ ASSERT_TRUE(svcId2 >= 0);
ASSERT_EQ(4, count);
celix_bundleContext_unregisterService(ctx, svcId1);
@@ -496,11 +496,11 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerTestWithProperties) {
opts.addWithProperties = add;
opts.removeWithProperties = remove;
long trackerId = celix_bundleContext_trackServicesWithOptions(ctx, &opts);
- ASSERT_TRUE(trackerId > 0);
+ ASSERT_TRUE(trackerId >= 0);
ASSERT_EQ(1, count);
long svcId2 = celix_bundleContext_registerService(ctx, (void*)0x200, "calc", nullptr);
- ASSERT_TRUE(svcId1 > 0);
+ ASSERT_TRUE(svcId1 >= 0);
ASSERT_EQ(2, count);
celix_bundleContext_unregisterService(ctx, svcId1);
@@ -535,11 +535,11 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerTestWithOwner) {
opts.addWithOwner = add;
opts.removeWithOwner = remove;
long trackerId = celix_bundleContext_trackServicesWithOptions(ctx, &opts);
- ASSERT_TRUE(trackerId > 0);
+ ASSERT_TRUE(trackerId >= 0);
ASSERT_EQ(1, count);
long svcId2 = celix_bundleContext_registerService(ctx, (void*)0x200, "calc", nullptr);
- ASSERT_TRUE(svcId1 > 0);
+ ASSERT_TRUE(svcId1 >= 0);
ASSERT_EQ(2, count);
celix_bundleContext_unregisterService(ctx, svcId1);
@@ -616,7 +616,7 @@ TEST_F(CelixBundleContextServicesTests, serviceTrackerWithRaceConditionTest) {
std::thread unregisterThread{[&]{
long id = -1;
std::unique_lock<std::mutex> lock(data.mutex);
- data.sync.wait(lock, [&]{return data.svcId > 0;});
+ data.sync.wait(lock, [&]{return data.svcId >= 0;});
id = data.svcId;
lock.unlock();
std::cout << "trying to unregister ... with id " << id << std::endl;
@@ -688,7 +688,7 @@ TEST_F(CelixBundleContextServicesTests, servicesTrackerSetTest) {
opts.filter.serviceName = "NA";
opts.set = set;
long trackerId = celix_bundleContext_trackServicesWithOptions(ctx, &opts);
- ASSERT_TRUE(trackerId > 0);
+ ASSERT_TRUE(trackerId >= 0);
//register svc3 should lead to second set call
properties_t *props3 = celix_properties_create();
diff --git a/libs/framework/include/service_registry.h b/libs/framework/include/service_registry.h
index 28d8afd..db93cb5 100644
--- a/libs/framework/include/service_registry.h
+++ b/libs/framework/include/service_registry.h
@@ -128,6 +128,12 @@ bool celix_serviceRegistry_getServiceInfo(
bool *factory);
+/**
+ * Returns the next svc id.
+ */
+long celix_serviceRegistry_nextSvcId(celix_service_registry_t* registry);
+
+
#ifdef __cplusplus
}
#endif
diff --git a/libs/framework/private/test/service_registry_test.cpp b/libs/framework/private/test/service_registry_test.cpp
index 9634537..40b51be 100644
--- a/libs/framework/private/test/service_registry_test.cpp
+++ b/libs/framework/private/test/service_registry_test.cpp
@@ -104,7 +104,7 @@ TEST(service_registry, create) {
serviceRegistry_create(framework, ®istry);
POINTERS_EQUAL(framework, registry->framework);
- UNSIGNED_LONGS_EQUAL(1UL, registry->currentServiceId);
+ UNSIGNED_LONGS_EQUAL(0L, registry->nextServiceId);
CHECK(registry->listenerHooks != NULL);
CHECK(registry->serviceReferences != NULL);
CHECK(registry->serviceRegistrations != NULL);
@@ -176,115 +176,6 @@ TEST(service_registry, getServicesInUse) {
hashMap_destroy(usages, false, false);
}
-TEST(service_registry, registerServiceNoProps) {
- service_registry_pt registry = NULL;
- framework_pt framework = (framework_pt) 0x01;
- serviceRegistry_create(framework, ®istry);
-
- bundle_pt bundle = (bundle_pt) 0x10;
- char * serviceName = my_strdup("service");
- void *service = (void *) 0x20;
- service_registration_pt reg = (service_registration_pt) 0x30;
-
- mock()
- .expectOneCall("serviceRegistration_create")
- .withParameterOfType("registry_callback_t", "callback", ®istry->callback)
- .withParameter("bundle", bundle)
- .withParameter("serviceName", serviceName)
- .withParameter("serviceId", 2)
- .withParameter("serviceObject", service)
- .withParameter("dictionary", (void *) NULL)
- .andReturnValue(reg);
-
- mock().expectNCalls(1,"serviceRegistration_getServiceId")
- .withParameter("registration", reg)
- .andReturnValue(42);
-
- service_registration_pt registration = NULL;
- serviceRegistry_registerService(registry, bundle, serviceName, service, NULL, ®istration);
- POINTERS_EQUAL(reg, registration);
-
- array_list_pt destroy_this = (array_list_pt) hashMap_remove(registry->serviceRegistrations, bundle);
- arrayList_destroy(destroy_this);
- serviceRegistry_destroy(registry);
- free(serviceName);
-}
-
-TEST(service_registry, registerServiceFactoryNoProps) {
- service_registry_pt registry = NULL;
- framework_pt framework = (framework_pt) 0x01;
- serviceRegistry_create(framework, ®istry);
-
- bundle_pt bundle = (bundle_pt) 0x10;
- char * serviceName = my_strdup("service");
- service_factory_pt factory = (service_factory_pt) malloc(sizeof(*factory));
- factory->handle = (void*) 0x20;
- service_registration_pt reg = (service_registration_pt) 0x40;
-
- mock()
- .expectOneCall("serviceRegistration_createServiceFactory")
- .withParameterOfType("registry_callback_t", "callback", ®istry->callback)
- .withParameter("bundle", bundle)
- .withParameter("serviceName", serviceName)
- .withParameter("serviceId", 2)
- .withParameter("serviceObject", factory)
- .withParameter("dictionary", (void *) NULL)
- .andReturnValue(reg);
-
- mock().expectNCalls(1,"serviceRegistration_getServiceId")
- .withParameter("registration", reg)
- .andReturnValue(42);
-
- service_registration_pt registration = NULL;
- serviceRegistry_registerServiceFactory(registry, bundle, serviceName, factory, NULL, ®istration);
- POINTERS_EQUAL(reg, registration);
-
- array_list_pt destroy_this = (array_list_pt) hashMap_remove(registry->serviceRegistrations, bundle);
- arrayList_destroy(destroy_this);
- serviceRegistry_destroy(registry);
- free(serviceName);
- free(factory);
-}
-
-TEST(service_registry, registerServiceListenerHook) {
- service_registry_pt registry = NULL;
- framework_pt framework = (framework_pt) 0x01;
- serviceRegistry_create(framework, ®istry);
-
- bundle_pt bundle = (bundle_pt) 0x10;
- char * serviceName = my_strdup(OSGI_FRAMEWORK_LISTENER_HOOK_SERVICE_NAME);
- void *service = (void *) 0x20;
- service_registration_pt reg = (service_registration_pt) 0x30;
- long svcId = 11;
-
- mock()
- .expectOneCall("serviceRegistration_create")
- .withParameterOfType("registry_callback_t", "callback", ®istry->callback)
- .withParameter("bundle", bundle)
- .withParameter("serviceName", serviceName)
- .withParameter("serviceId", 2)
- .withParameter("serviceObject", service)
- .withParameter("dictionary", (void *) NULL)
- .andReturnValue(reg);
-
- mock().expectNCalls(2,"serviceRegistration_getServiceId")
- .withParameter("registration", reg)
- .andReturnValue(svcId);
-
- service_registration_pt registration = NULL;
- serviceRegistry_registerService(registry, bundle, serviceName, service, NULL, ®istration);
- POINTERS_EQUAL(reg, registration);
- LONGS_EQUAL(1, arrayList_size(registry->listenerHooks));
-
- auto* entry = (celix_service_registry_listener_hook_entry_t*)celix_arrayList_get(registry->listenerHooks, 0);
- POINTERS_EQUAL(svcId, entry->svcId);
-
- //cleanup
- array_list_pt destroy_this = (array_list_pt) hashMap_remove(registry->serviceRegistrations, bundle);
- arrayList_destroy(destroy_this);
- serviceRegistry_destroy(registry);
- free(serviceName);
-}
TEST(service_registry, unregisterService) {
service_registry_pt registry = NULL;
diff --git a/libs/framework/src/service_registry.c b/libs/framework/src/service_registry.c
index 8a8bd94..154f4e6 100644
--- a/libs/framework/src/service_registry.c
+++ b/libs/framework/src/service_registry.c
@@ -79,7 +79,7 @@ celix_status_t serviceRegistry_create(framework_pt framework, service_registry_p
reg->serviceRegistrations = hashMap_create(NULL, NULL, NULL, NULL);
reg->framework = framework;
- reg->currentServiceId = 1UL;
+ reg->nextServiceId = 0L;
reg->serviceReferences = hashMap_create(NULL, NULL, NULL, NULL);
reg->checkDeletedReferences = CHECK_DELETED_REFERENCES;
@@ -207,17 +207,17 @@ celix_status_t serviceRegistry_registerServiceFactory(service_registry_pt regist
static celix_status_t serviceRegistry_registerServiceInternal(service_registry_pt registry, bundle_pt bundle, const char* serviceName, const void * serviceObject, properties_pt dictionary, enum celix_service_type svcType, service_registration_pt *registration) {
array_list_pt regs;
+ long svcId = celix_serviceRegistry_nextSvcId(registry);
if (svcType == CELIX_DEPRECATED_FACTORY_SERVICE) {
*registration = serviceRegistration_createServiceFactory(registry->callback, bundle, serviceName,
- ++registry->currentServiceId, serviceObject,
+ svcId, serviceObject,
dictionary);
} else if (svcType == CELIX_FACTORY_SERVICE) {
- *registration = celix_serviceRegistration_createServiceFactory(registry->callback, bundle, serviceName, ++registry->currentServiceId, (celix_service_factory_t*)serviceObject, dictionary);
+ *registration = celix_serviceRegistration_createServiceFactory(registry->callback, bundle, serviceName, svcId, (celix_service_factory_t*)serviceObject, dictionary);
} else { //plain
- *registration = serviceRegistration_create(registry->callback, bundle, serviceName, ++registry->currentServiceId, serviceObject, dictionary);
+ *registration = serviceRegistration_create(registry->callback, bundle, serviceName, svcId, serviceObject, dictionary);
}
- long svcId = serviceRegistration_getServiceId(*registration);
//printf("Registering service %li with name %s\n", svcId, serviceName);
serviceRegistry_addHooks(registry, serviceName, serviceObject, *registration);
@@ -1215,4 +1215,9 @@ static void celix_waitForPendingRegisteredEvents(celix_service_registry_t *regis
count = (long)hashMap_get(registry->pendingRegisterEvents.map, (void*)svcId);
}
celixThreadMutex_unlock(®istry->pendingRegisterEvents.mutex);
+}
+
+long celix_serviceRegistry_nextSvcId(celix_service_registry_t* registry) {
+ long scvId = __atomic_fetch_add(®istry->nextServiceId, 1, __ATOMIC_SEQ_CST);
+ return scvId;
}
\ No newline at end of file
diff --git a/libs/framework/src/service_registry_private.h b/libs/framework/src/service_registry_private.h
index 3a4cd01..fb8e37f 100644
--- a/libs/framework/src/service_registry_private.h
+++ b/libs/framework/src/service_registry_private.h
@@ -44,7 +44,7 @@ struct celix_serviceRegistry {
bool checkDeletedReferences; //If enabled. check if provided service references are still valid
hash_map_t *deletedServiceReferences; //key = ref pointer, value = bool
- unsigned long currentServiceId;
+ long nextServiceId;
celix_array_list_t *listenerHooks; //celix_service_registry_listener_hook_entry_t*
celix_array_list_t *serviceListeners; //celix_service_registry_service_listener_entry_t*
diff --git a/libs/framework/src/service_tracker.c b/libs/framework/src/service_tracker.c
index 20e9e60..6003757 100644
--- a/libs/framework/src/service_tracker.c
+++ b/libs/framework/src/service_tracker.c
@@ -510,7 +510,7 @@ static void serviceTracker_checkAndInvokeSetService(void *handle, void *highestS
} else {
svcId = celix_properties_getAsLong(props, OSGI_FRAMEWORK_SERVICE_ID, -1);
}
- if (svcId > 0) {
+ if (svcId >= 0) {
celixThreadMutex_lock(&instance->mutex);
if (instance->currentHighestServiceId != svcId) {
instance->currentHighestServiceId = svcId;