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 <[email protected]> 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;
