[celix] branch develop updated: Fixes an issue where get a service from a service factory can lead to a deadlock

[pnoltes]

This is an automated email from the ASF dual-hosted git repository.

pnoltes pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/celix.git


The following commit(s) were added to refs/heads/develop by this push:
     new 52253e7  Fixes an issue where get a service from a service factory can 
lead to a deadlock
52253e7 is described below

commit 52253e72605238388a385d42da181b63ea1b516d
Author: Pepijn Noltes <pepijnnol...@gmail.com>
AuthorDate: Fri May 24 17:32:19 2019 +0200

    Fixes an issue where get a service from a service factory can lead to a 
deadlock
---
 .../src/simple_consumer_example.c                  |  2 +-
 .../private/test/service_registry_test.cpp         |  3 ++
 libs/framework/src/service_registry.c              | 34 +++++++++++++---------
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git 
a/examples/celix-examples/services_example_c/src/simple_consumer_example.c 
b/examples/celix-examples/services_example_c/src/simple_consumer_example.c
index db1a4aa..e078f56 100644
--- a/examples/celix-examples/services_example_c/src/simple_consumer_example.c
+++ b/examples/celix-examples/services_example_c/src/simple_consumer_example.c
@@ -66,4 +66,4 @@ static celix_status_t activator_stop(activator_data_t *data, 
celix_bundle_contex
     return CELIX_SUCCESS;
 }
 
-CELIX_GEN_BUNDLE_ACTIVATOR(activator_data_t, activator_start, activator_stop)
\ No newline at end of file
+CELIX_GEN_BUNDLE_ACTIVATOR(activator_data_t, activator_start, activator_stop)
diff --git a/libs/framework/private/test/service_registry_test.cpp 
b/libs/framework/private/test/service_registry_test.cpp
index 6f8aabc..a0ee6eb 100644
--- a/libs/framework/private/test/service_registry_test.cpp
+++ b/libs/framework/private/test/service_registry_test.cpp
@@ -871,6 +871,9 @@ TEST(service_registry, getService) {
                .expectOneCall("serviceRegistration_isValid")
                .withParameter("registration", registration)
                .andReturnValue(false);
+       mock()
+                       .expectOneCall("serviceRegistration_retain")
+                       .withParameter("registration", registration);
 
        actual = (void*) 0x666;//generic non null pointer value
        serviceRegistry_getService(registry, bundle, reference, &actual);
diff --git a/libs/framework/src/service_registry.c 
b/libs/framework/src/service_registry.c
index ea7effd..3516342 100644
--- a/libs/framework/src/service_registry.c
+++ b/libs/framework/src/service_registry.c
@@ -656,34 +656,40 @@ celix_status_t 
serviceRegistry_getService(service_registry_pt registry, bundle_p
        service_registration_pt registration = NULL;
     size_t count = 0;
     const void *service = NULL;
+    bool valid = false;
     reference_status_t refStatus;
 
 
-
     celixThreadRwlock_readLock(&registry->lock);
     serviceRegistry_checkReference(registry, reference, &refStatus);
     if (refStatus == REF_ACTIVE) {
         serviceReference_getServiceRegistration(reference, &registration);
-
-        if (serviceRegistration_isValid(registration)) {
+        valid = serviceRegistration_isValid(registration);
+        if (valid) {
+            serviceRegistration_retain(registration);
             serviceReference_increaseUsage(reference, &count);
-            if (count == 1) {
-                serviceRegistration_getService(registration, bundle, &service);
-                serviceReference_setService(reference, service);
-            }
-
-            /* NOTE the out argument of sr_getService should be 'const void**'
-               To ensure backwards compatibility a cast is made instead.
-            */
-            serviceReference_getService(reference, (void **)out);
         } else {
             *out = NULL; //invalid service registration
         }
+    }
+    celixThreadRwlock_unlock(&registry->lock);
+
+    if (valid && refStatus == REF_ACTIVE) {
+        if (count == 1) {
+            serviceRegistration_getService(registration, bundle, &service);
+            serviceReference_setService(reference, service);
+        }
+
+        /* NOTE the out argument of sr_getService should be 'const void**'
+           To ensure backwards compatibility a cast is made instead.
+        */
+        serviceReference_getService(reference, (void **)out);
     } else {
-        serviceRegistry_logIllegalReference(registry, reference, refStatus);
+        if (refStatus != REF_ACTIVE) {
+            serviceRegistry_logIllegalReference(registry, reference, 
refStatus);
+        }
         status = CELIX_BUNDLE_EXCEPTION;
     }
-    celixThreadRwlock_unlock(&registry->lock);
 
        return status;
 }