Fix race condition when concurrently adding to SCRExtensionRegistry (#…

…870) * Fix race condition when concurrently adding to SCRExtensionRegistry Fixed a race that can happen when multiple threads are trying to add to the extension registry container. Fixed a bug with the tests for SCRExtensionRegsitry class never being compiled and run. Signed-off-by: The MathWorks, Inc. <jdicleme@mathworks.com>
CppMicroServices · Jun 21, 2023 · 44e1481 · 44e1481 · github-actions · Jun 21, 2023
1 parent f244220
commit 44e1481
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 110 deletions.
diff --git a/compendium/DeclarativeServices/src/SCRExtensionRegistry.cpp b/compendium/DeclarativeServices/src/SCRExtensionRegistry.cpp
@@ -25,7 +25,7 @@ namespace cppmicroservices
 {
     namespace scrimpl
     {
-        SCRExtensionRegistry::SCRExtensionRegistry(std::shared_ptr<SCRLogger> logger)
+        SCRExtensionRegistry::SCRExtensionRegistry(std::shared_ptr<cppmicroservices::logservice::LogService> logger)
             : logger(std::move(logger))
         {
             if (!(this->logger))
@@ -49,11 +49,13 @@ namespace cppmicroservices
         void
         SCRExtensionRegistry::Add(long bundleId, std::shared_ptr<SCRBundleExtension> extension)
         {
-            if (!extension) {
+            if (!extension)
+            {
                 throw std::invalid_argument("SCRExtensionRegistry::Add invalid extension");	
             }
-            if (extensionRegistry.find(bundleId) == extensionRegistry.end()){
-                std::lock_guard<std::mutex> l(extensionRegMutex);
+            std::lock_guard<std::mutex> l(extensionRegMutex);
+            if (extensionRegistry.find(bundleId) == extensionRegistry.end())
+            {
                 extensionRegistry.insert(std::make_pair(bundleId, std::move(extension)));
             }
         }

diff --git a/compendium/DeclarativeServices/src/SCRExtensionRegistry.hpp b/compendium/DeclarativeServices/src/SCRExtensionRegistry.hpp
@@ -24,7 +24,7 @@
 #define __CPPMICROSERVICES_SCRIMPL_SCREXTENSIONREGISTRY_HPP__
 
 #include "SCRBundleExtension.hpp"
-#include "SCRLogger.hpp"
+#include "cppmicroservices/logservice/LogService.hpp"
 
 #include <map>
 #include <memory>
@@ -46,7 +46,7 @@ namespace cppmicroservices
             /**
              * @throws std::invalid_argument exception if any of the params is a nullptr
              */
-            SCRExtensionRegistry(std::shared_ptr<SCRLogger> logger);
+            SCRExtensionRegistry(std::shared_ptr<cppmicroservices::logservice::LogService> logger);
 
             SCRExtensionRegistry(SCRExtensionRegistry const&) = delete;
             SCRExtensionRegistry(SCRExtensionRegistry&&) = delete;
@@ -93,7 +93,7 @@ namespace cppmicroservices
             void Clear();
 
           private:
-            std::shared_ptr<SCRLogger> logger;
+            std::shared_ptr<cppmicroservices::logservice::LogService> logger;
             std::mutex extensionRegMutex;  //protects the extensionRegistry
             std::unordered_map<long,std::shared_ptr<SCRBundleExtension>> extensionRegistry;
         };

diff --git a/compendium/DeclarativeServices/test/gtest/CMakeLists.txt b/compendium/DeclarativeServices/test/gtest/CMakeLists.txt
@@ -77,6 +77,7 @@ set(_declarativeservices_tests
   TestReferenceSelfSatisfyDeadLock.cpp
   TestRegistrationManager.cpp
   TestSCRBundleExtension.cpp
+  TestSCRExtensionRegistry.cpp
   TestServiceComponentRuntimeImpl.cpp
   TestServiceMetadataParserV1.cpp
   TestSetConfiguration.cpp

diff --git a/compendium/DeclarativeServices/test/gtest/TestSCRExtensionRegistry.cpp b/compendium/DeclarativeServices/test/gtest/TestSCRExtensionRegistry.cpp
@@ -30,6 +30,7 @@
 #include <cppmicroservices/Framework.h>
 #include <cppmicroservices/FrameworkEvent.h>
 #include <cppmicroservices/FrameworkFactory.h>
+#include "ConcurrencyTestUtil.hpp"
 #include "gmock/gmock.h"
 #include "Mocks.hpp"
 
@@ -73,66 +74,6 @@ namespace cppmicroservices
                 return framework;
             }
 
-            void
-            ConcurrentInvoke(std::vector<cppmicroservices::Bundle> allBundles, bool addOperation)
-            {
-                std::promise<void> go;
-                std::shared_future<void> ready(go.get_future());
-                int numCalls = allBundles.size();
-                std::vector<std::promise<void>> readies(numCalls);
-                std::vector<std::future<void>> bundle_result(numCalls);
-                try
-                {
-                    if (addOperation)
-                    {
-                        for (int i = 0; i < numCalls; i++)
-                        {
-
-                            bundle_result[i]
-                                = std::async(std::launch::async,
-                                             [&readies, &ready, &allBundles, this, i]()
-                                             {
-                                                 readies[i].set_value();
-                                                 ready.wait();
-                                                 auto ba = std::make_shared<SCRBundleExtension>(allBundles[i],
-                                                                                                this->fakeRegistry,
-                                                                                                this->logger,
-                                                                                                this->notifier);
-                                                 extRegistry->Add(allBundles[i].GetBundleId(), ba);
-                                             });
-                        }
-                    }
-                    else // remove operation
-                    {
-                        for (int i = 0; i < numCalls; i++)
-                        {
-                            bundle_result[i] = std::async(std::launch::async,
-                                                          [&readies, &ready, &allBundles, this, i]()
-                                                          {
-                                                              readies[i].set_value();
-                                                              ready.wait();
-                                                              this->extRegistry->Remove(allBundles[i].GetBundleId());
-                                                          });
-                        }
-                    }
-
-                    for (int i = 0; i < numCalls; i++)
-                    {
-                        readies[i].get_future().wait();
-                    }
-                    go.set_value();
-                    for (int i = 0; i < numCalls; i++)
-                    {
-                        bundle_result[i].wait();
-                    }
-                }
-                catch (std::exception const& e)
-                {
-                    EXPECT_TRUE(false) << "Error: exception received ... " << e.what() << std::endl;
-                    go.set_value();
-                    throw std::current_exception();
-                }
-            }
           protected:
             cppmicroservices::Framework framework;
             std::shared_ptr<ComponentRegistry> fakeRegistry;
@@ -185,78 +126,71 @@ namespace cppmicroservices
             asyncWorkService->StopTracking();
             fakeRegistry->Clear();
         }
-        // Test to test concurrent additions of bundle extensions to the SCRExtensionRegistry and
+        // Test concurrent additions of bundle extensions to the SCRExtensionRegistry and
         // concurrent removals.
         TEST_F(SCRExtensionRegistryTest, VerifyConcurrentAddRemove)
         {
-            int count = 4;
-            test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI1");
-            test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI2");
-            test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI3");
-            test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI5");
-
+            constexpr int fakeBundleCount{100};
             auto bundleContext = GetFramework().GetBundleContext();
-            auto allBundles = bundleContext.GetBundles();
-            ASSERT_TRUE(allBundles.size() > count) << "All bundles not installed.";
+            // This test doesn't require unique or even functional Bundle objects. Use the
+            // same bundle object for the purpose of testing thread safety of the SCRExtensionRegistry
+            // methods.
+            const auto bundle = test::InstallAndStartBundle(bundleContext, "TestBundleDSTOI1");
 
             // Add a bundle extension object for each bundle in the allBundles vector to the 
             // extension registry
-            ConcurrentInvoke(allBundles, true);           
-            for (auto const& item : allBundles) {
-                ASSERT_TRUE(extRegistry->Find(item.GetBundleId()))
-                    << "bundle " << item.GetSymbolicName() << " not found.";
-            }
+            std::function<bool()> addFunc = [&]() -> bool {
+                for(int fakeBundleId = 0; fakeBundleId <= fakeBundleCount; ++fakeBundleId) 
+                {
+                    extRegistry->Add(fakeBundleId, std::make_shared<SCRBundleExtension>(bundle, fakeRegistry, logger, notifier));
+                }
+                return true;
+                };
+
+            ASSERT_NO_THROW((void)ConcurrentInvoke(std::move(addFunc)));
 
             // Remove the bundle extension for all bundles in the allBundles vector from 
             // the extension registry.
-            ConcurrentInvoke(allBundles, false);
-            for (auto const& item : allBundles)
-            {
-                ASSERT_TRUE(!extRegistry->Find(item.GetBundleId()))
-                    << "bundle " << item.GetSymbolicName() << " should have been removed.";
-            }
+            std::function<bool()> removeFunc = [&]() -> bool {
+                for(int fakeBundleId = 0; fakeBundleId <= fakeBundleCount; ++fakeBundleId)
+                {
+                    extRegistry->Remove(fakeBundleId);
+                }
+                return true;
+                };
+
+            ASSERT_NO_THROW((void)ConcurrentInvoke(std::move(removeFunc)));
+
             asyncWorkService->StopTracking();
             fakeRegistry->Clear();
 
         }
 
-        //Mock SCRExtensionRegistry class used by the testException class
-        class MockSCRExtensionRegistry : public cppmicroservices::scrimpl::SCRExtensionRegistry
-        {
-          public:
-            MockSCRExtensionRegistry(std::shared_ptr<SCRLogger> const& logger)
-                : cppmicroservices::scrimpl::SCRExtensionRegistry (logger)
-            {
-            }
-            virtual ~MockSCRExtensionRegistry() = default;    
-            MOCK_METHOD1(Find, std::shared_ptr<SCRBundleExtension>(long bundleId));
-        }; 
-
         // Tests the CreateFactoryComponent method of the ConfigurationNotifier class. If the bundle extension 
         // cannot be found in the SCRExtensionRegistry when creating a factory component then an
         // exception will be logged.
-        TEST_F(SCRExtensionRegistryTest, testException)
+        TEST_F(SCRExtensionRegistryTest, testCreateFactoryComponentFailure)
         {
             auto bundle = test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI1");
             ASSERT_TRUE(static_cast<bool>(bundle));
-            auto mockExtRegistry = std::make_shared<MockSCRExtensionRegistry>(logger);
             auto mockLogger = std::make_shared<MockLogger>();
             auto notifier = std::make_shared<ConfigurationNotifier>(GetFramework().GetBundleContext(),
                                                                     mockLogger,
                                                                     asyncWorkService,
-                                                                    mockExtRegistry);
-
-            EXPECT_CALL(*mockExtRegistry, Find (bundle.GetBundleId())).Times(1).WillOnce(::testing::Return(nullptr));
-            EXPECT_CALL(*(mockLogger.get()),
-                        Log(cppmicroservices::logservice::SeverityLevel::LOG_ERROR, testing::_, testing::_))
+                                                                    extRegistry);
+
+            EXPECT_CALL(*mockLogger, Log(cppmicroservices::logservice::SeverityLevel::LOG_ERROR, testing::_))
                 .Times(1);
-            auto mockMetadata = std::make_shared<metadata::ComponentMetadata>();
-
+            auto mockMetadata = std::make_shared<metadata::ComponentMetadata>(); 
 
             std::shared_ptr<ComponentConfigurationImpl> compConfigImpl
-                = std::make_shared<SingletonComponentConfigurationImpl>(mockMetadata, bundle, fakeRegistry, logger, notifier);
+                = std::make_shared<SingletonComponentConfigurationImpl>(mockMetadata, bundle, fakeRegistry, mockLogger, notifier);
             std::string pid { 123 };
-            EXPECT_NO_THROW({ notifier->CreateFactoryComponent(pid, compConfigImpl); }); 
+            EXPECT_NO_THROW({ notifier->CreateFactoryComponent(pid, compConfigImpl); });
+
+            asyncWorkService->StopTracking();
+            fakeRegistry->Clear();
+            compConfigImpl->Deactivate();
          }
     } // namespace scrimpl
 } // namespace cppmicroservices