Remove deadlock in config notification (CppMicroServices#1040) (CppMicroServices#1174)

* added test to verify that update call from inside modified no longer causes deadlock

* memory leaks... no idea why they are happening but 99% sure this is a false positive

* fix false leak

* respond to jeff comments

* respond to conor
's comment

---------

Co-authored-by: tcormackMW <113473781+tcormackMW@users.noreply.github.com>
Co-authored-by: Toby Cormack <tcormack@mathworks.com>
Co-authored-by: Jeff DiClemente <jeffdiclemente@users.noreply.github.com>

Author: 3 people

compendium/DeclarativeServices/src/manager/ConfigurationNotifier.cpp

@@ -185,8 +185,6 @@ namespace cppmicroservices
                                                   cppmicroservices::service::cm::ConfigurationEventType type,
                                                   std::shared_ptr<cppmicroservices::AnyMap> properties)
         {
-            // lock held throughout to guarantee ordering for calls to this function
-            std::lock_guard<std::mutex> l(notificationOrderingLock);
             ConfigChangeNotification notification
                 = ConfigChangeNotification(pid, std::move(properties), std::move(type));
             std::vector<Listener> callbacks;

compendium/DeclarativeServices/src/manager/ConfigurationNotifier.hpp

@@ -102,11 +102,9 @@ namespace cppmicroservices
             void NotifyAllListeners(std::string const& pid,
                                     cppmicroservices::service::cm::ConfigurationEventType type,
                                     std::shared_ptr<cppmicroservices::AnyMap> properties);
-
             std::shared_ptr<ComponentFactoryImpl> GetComponentFactory();
             void LogInvalidDynamicTargetInProperties(cppmicroservices::AnyMap const& properties,
-                                  std::shared_ptr<ComponentConfigurationImpl> mgr) const noexcept;
-
+                                                     std::shared_ptr<ComponentConfigurationImpl> mgr) const noexcept;
           private:
             using TokenMap = std::unordered_map<ListenerTokenId, Listener>;
 
@@ -119,9 +117,8 @@ namespace cppmicroservices
 
             std::shared_ptr<cppmicroservices::logservice::LogService> logger;
             std::shared_ptr<ComponentFactoryImpl> componentFactory;
-            std::mutex notificationOrderingLock;
-         };
+        };
 
     } // namespace scrimpl
 } // namespace cppmicroservices
-#endif //__CPPMICROSERVICES_SCRIMPL_CONFIGURATIONNOTIFIER_HPP__
+#endif // __CPPMICROSERVICES_SCRIMPL_CONFIGURATIONNOTIFIER_HPP__

compendium/DeclarativeServices/test/TestUtils.cpp

@@ -275,4 +275,43 @@ namespace test
 #endif
     }
 
+    AsyncWorkServiceThreadPool::AsyncWorkServiceThreadPool(int nThreads) : cppmicroservices::async::AsyncWorkService(), threadpool(std::make_shared<boost::asio::thread_pool>(nThreads))
+    {
+    }
+
+    AsyncWorkServiceThreadPool::~AsyncWorkServiceThreadPool()
+    {
+        try
+        {
+            if (threadpool)
+            {
+                try
+                {
+                    threadpool->join();
+                }
+                catch (...)
+                {
+                    //
+                }
+            }
+        }
+        catch (...)
+        {
+            //
+        }
+    }
+
+    void
+    AsyncWorkServiceThreadPool::post(std::packaged_task<void()>&& task)
+    {
+        using Sig = void();
+        using Result = boost::asio::async_result<decltype(task), Sig>;
+        using Handler = typename Result::completion_handler_type;
+
+        Handler handler{std::move(task)};
+        Result result(handler);
+
+        boost::asio::post(threadpool->get_executor(), [handler = std::move(handler)]() mutable { handler(); });
+    }
+
 } // namespace test

compendium/DeclarativeServices/test/TestUtils.hpp

@@ -26,7 +26,11 @@
 #include <cppmicroservices/Bundle.h>
 #include <cppmicroservices/BundleContext.h>
 #include <cppmicroservices/ServiceReference.h>
-
+#include "boost/asio/async_result.hpp"
+#include "boost/asio/packaged_task.hpp"
+#include "boost/asio/post.hpp"
+#include "boost/asio/thread_pool.hpp"
+#include <cppmicroservices/asyncworkservice/AsyncWorkService.hpp>
 #include <random>
 #include <string>
 
@@ -103,6 +107,20 @@ namespace test
      */
     bool isBundleLoadedInThisProcess(std::string bundleName);
 
+    class AsyncWorkServiceThreadPool : public cppmicroservices::async::AsyncWorkService
+    {
+      public:
+        AsyncWorkServiceThreadPool(int nThreads);
+
+        ~AsyncWorkServiceThreadPool() override;
+
+        void
+        post(std::packaged_task<void()>&& task) override;
+
+      private:
+        std::shared_ptr<boost::asio::thread_pool> threadpool;
+    };
+
 } // namespace test
 
 #endif /* TestUtils_hpp */

compendium/DeclarativeServices/test/gtest/CMakeLists.txt

@@ -206,6 +206,7 @@ set(_test_bundles
   TestBundleDSCA07
   TestBundleDSCA08
   TestBundleDSCA09
+  TestBundleDSCA10
   TestBundleDSCA12
   TestBundleDSCA16
   TestBundleDSCA20

compendium/DeclarativeServices/test/gtest/ImportTestBundles.cpp

@@ -37,6 +37,7 @@ CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA06)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA07)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA08)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA09)
+CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA10)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA12)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA16)
 CPPMICROSERVICES_INITIALIZE_STATIC_BUNDLE(TestBundleDSCA20)

compendium/DeclarativeServices/test/gtest/TestAsyncWorkService.cpp

@@ -34,10 +34,7 @@
 #include "Mocks.hpp"
 #include "TestFixture.hpp"
 #include "TestInterfaces/Interfaces.hpp"
-#include "boost/asio/async_result.hpp"
-#include "boost/asio/packaged_task.hpp"
-#include "boost/asio/post.hpp"
-#include "boost/asio/thread_pool.hpp"
+
 #include "cppmicroservices/servicecomponent/ComponentConstants.hpp"
 #include "cppmicroservices/servicecomponent/runtime/ServiceComponentRuntime.hpp"
 
@@ -80,53 +77,6 @@ namespace test
         cppmicroservices::Framework framework;
     };
 
-    class AsyncWorkServiceThreadPool : public cppmicroservices::async::AsyncWorkService
-    {
-      public:
-        AsyncWorkServiceThreadPool(int nThreads) : cppmicroservices::async::AsyncWorkService()
-        {
-            threadpool = std::make_shared<boost::asio::thread_pool>(nThreads);
-        }
-
-        ~AsyncWorkServiceThreadPool() override
-        {
-            try
-            {
-                if (threadpool)
-                {
-                    try
-                    {
-                        threadpool->join();
-                    }
-                    catch (...)
-                    {
-                        //
-                    }
-                }
-            }
-            catch (...)
-            {
-                //
-            }
-        }
-
-        void
-        post(std::packaged_task<void()>&& task) override
-        {
-            using Sig = void();
-            using Result = boost::asio::async_result<decltype(task), Sig>;
-            using Handler = typename Result::completion_handler_type;
-
-            Handler handler(std::forward<decltype(task)>(task));
-            Result result(handler);
-
-            boost::asio::post(threadpool->get_executor(), [handler = std::move(handler)]() mutable { handler(); });
-        }
-
-      private:
-        std::shared_ptr<boost::asio::thread_pool> threadpool;
-    };
-
     TEST_F(tGenericDSSuite, TestAsyncWorkServiceWithoutUserService)
     {
         std::shared_ptr<cppmicroservices::scrimpl::SCRLogger> logger
@@ -304,7 +254,7 @@ namespace test
 
             auto reg = ctx.RegisterService<cppmicroservices::async::AsyncWorkService>(param);
 
-            for (const auto& bundleName : bundlesToInstall)
+            for (auto const& bundleName : bundlesToInstall)
             {
                 installedBundles.emplace_back(::test::InstallAndStartBundle(ctx, bundleName));
             }
@@ -343,7 +293,7 @@ namespace test
         auto configInstance = configuration->GetPid();
 
         cppmicroservices::AnyMap props({
-            {"uniqueProp", std::string("instance1")}
+            { "uniqueProp", std::string("instance1") }
         });
 
         auto fut = configuration->Update(props);
@@ -391,7 +341,7 @@ namespace test
                 auto configInstance = configuration->GetPid();
 
                 cppmicroservices::AnyMap props({
-                    {"uniqueProp", std::string("instance1")}
+                    { "uniqueProp", std::string("instance1") }
                 });
 
                 auto fut = configuration->Update(props);
@@ -409,14 +359,14 @@ namespace test
                 auto configInstance = configuration->GetPid();
 
                 cppmicroservices::AnyMap props({
-                    {"uniqueProp", std::string("instance1")}
+                    { "uniqueProp", std::string("instance1") }
                 });
 
                 auto fut = configuration->SafeUpdate(props);
                 fut->get();
 
                 cppmicroservices::AnyMap props1({
-                    {"uniqueProp", std::string("instance2")}
+                    { "uniqueProp", std::string("instance2") }
                 });
                 auto fut1 = configuration->SafeUpdateIfDifferent(props1);
                 fut1.second->get();
@@ -457,7 +407,7 @@ namespace test
                 auto configInstance = configuration->GetPid();
 
                 cppmicroservices::AnyMap props({
-                    {"uniqueProp", std::string("instance1")}
+                    { "uniqueProp", std::string("instance1") }
                 });
 
                 auto fut = configuration->SafeUpdate(props);
@@ -471,4 +421,51 @@ namespace test
         fut.get();
     }
 
+    /**
+     * Verify that a service's configuration can be updated from with a 'Modified' callback without
+     * deadlocking the framework trying to get the lock in configNotifier
+     */
+    TEST_F(TestAsyncWorkServiceEndToEnd, testUpdateConfigFromWithinModifiedCallback)
+    {
+        auto const& param = std::make_shared<AsyncWorkServiceThreadPool>(10);
+        auto context = framework.GetBundleContext();
+
+        // ASYNCWORKSERVICE
+        auto reg = context.RegisterService<cppmicroservices::async::AsyncWorkService>(param);
+        US_UNUSED(reg);
+
+        // CA
+        ::test::InstallAndStartConfigAdmin(context);
+
+        //  CA Service
+        auto CARef = context.GetServiceReference<cppmicroservices::service::cm::ConfigurationAdmin>();
+        auto configAdminService = context.GetService<cppmicroservices::service::cm::ConfigurationAdmin>(CARef);
+        ASSERT_TRUE(configAdminService) << "GetService failed for ConfigurationAdmin";
+
+        // Start bundle
+        std::string componentName = "sample::ServiceComponentCA10";
+
+        auto testBundle = ::test::InstallAndStartBundle(context, "TestBundleDSCA10");
+        ::test::InstallAndStartBundle(context, "TestBundleDSCA03");
+
+        auto configObject = configAdminService->GetConfiguration(componentName);
+
+        auto props = configObject->GetProperties();
+        auto configObject2 = configAdminService->GetConfiguration("sample::ServiceComponentCA02");
+
+        props["uniqueProp"] = std::string("UNIQUE");
+        auto fut = configObject->Update(props);
+        fut.get();
+        fut = configObject2->Update(props);
+        fut.get();
+        props["configID"] = std::string("sample::ServiceComponentCA03");
+        fut = configObject->Update(props);
+        fut.get();
+
+        // GetService to make component active
+        auto serviceRef = context.GetServiceReference<test::CAInterface>();
+        auto service = context.GetService<test::CAInterface>(serviceRef);
+        ASSERT_TRUE(service) << "GetService failed for CAInterface";
+    }
+
 }; // namespace test

compendium/DeclarativeServices/test/gtest/TestFixture.hpp

@@ -109,6 +109,7 @@ namespace test
             test::InstallLib(context, "TestBundleDSCA07");
             test::InstallLib(context, "TestBundleDSCA08");
             test::InstallLib(context, "TestBundleDSCA09");
+            test::InstallLib(context, "TestBundleDSCA10");
             test::InstallLib(context, "TestBundleDSCA12");
             test::InstallLib(context, "TestBundleDSCA16");
             test::InstallLib(context, "TestBundleDSCA20");

compendium/DeclarativeServices/test/gtest/TestUpdateConfiguration.cpp

@@ -22,7 +22,6 @@
 
 #include "TestFixture.hpp"
 #include "gtest/gtest.h"
-
 #include "cppmicroservices/Constants.h"
 #include "cppmicroservices/ServiceObjects.h"
 
@@ -50,7 +49,7 @@ namespace test
         // starting the bundle which defines the service.
         auto configuration = configAdminService->GetConfiguration("sample::ServiceComponentCA02");
         configuration->UpdateIfDifferent(std::unordered_map<std::string, cppmicroservices::Any> {
-            {"foo", true}
+            { "foo", true }
         });
 
         // Install and start the bundle which has the service
@@ -95,7 +94,7 @@ namespace test
                              ready.wait();
                              auto configuration = configAdminService->GetConfiguration("sample::ServiceComponentCA02");
                              configuration->UpdateIfDifferent(std::unordered_map<std::string, cppmicroservices::Any> {
-                                 {"foo", true}
+                                 { "foo", true }
                              });
                          });
 
@@ -149,7 +148,7 @@ namespace test
 
         configuration
             ->UpdateIfDifferent(std::unordered_map<std::string, cppmicroservices::Any> {
-                {"foo", true}
+                { "foo", true }
         })
             .second.get();
 
@@ -245,6 +244,50 @@ namespace test
 
         testBundle.Stop();
     }
+
+    /*
+     * Verify that Factory Components will not be registered even if an incorrect Configuration matching exactly Factory
+     * Configuration PID is initialized.
+     */
+    TEST_F(tServiceComponent, testErroneousConfigurationInjection)
+    {
+        std::string const configName = "my.dependency.pid";
+
+        // Install and start the bundle containing the configuration object and the
+        // service which is dependent on the configuration object.
+        std::string componentName = "sample::ServiceComponentCA28";
+        cppmicroservices::Bundle testBundle = StartTestBundle("TestBundleDSCA28");
+        ASSERT_TRUE(testBundle);
+
+        auto configAdminService = GetInstance<cppmicroservices::service::cm::ConfigurationAdmin>();
+        ASSERT_TRUE(configAdminService) << "GetService failed for ConfigurationAdmin.";
+
+        // GetService to make component active
+        auto invalidService = GetInstance<test::CAInterface>();
+        ASSERT_FALSE(invalidService) << "Factory component itself should not be registered";
+
+        auto uniqueVal0 = 1;
+        auto configuration0 = configAdminService->GetConfiguration(configName);
+        cppmicroservices::AnyMap props0 { cppmicroservices::AnyMap::UNORDERED_MAP_CASEINSENSITIVE_KEYS,
+                                          { { "uniqueKey", uniqueVal0 } } };
+
+        auto fut0 = configuration0->Update(props0);
+        ASSERT_FALSE(GetInstance<test::CAInterface>())
+            << "Factory component should not be registered even with config matching exactly (with no ~)";
+
+        auto uniqueVal1 = 6;
+        auto configuration1 = configAdminService->GetConfiguration(configName + "~1");
+        cppmicroservices::AnyMap props1 { cppmicroservices::AnyMap::UNORDERED_MAP_CASEINSENSITIVE_KEYS,
+                                          { { "uniqueKey", uniqueVal1 } } };
+
+        auto fut1 = configuration1->Update(props1);
+        fut1.wait();
+        auto service0 = GetInstance<test::CAInterface>();
+        ASSERT_TRUE(service0) << "Factory instance should be created when a '~' configuration is added";
+        ASSERT_TRUE(service0->GetProperties().find("uniqueKey")->second == 6);
+
+        testBundle.Stop();
+    }
     /*
      * Tests that if a configuration object is defined in the manifest.json file
      * and the same configuration object is also defined programmatically before the service