Ensure ~SCRBundleExtension does not throw (#761)

* Ensure ~SCRBundleExtension does not throw It's possible for the bundle to be stopped before ~SCRBundleExtension accesses the bundle, causing an exception to be thrown and an abort. This change ensures the destructor does not throw. * Fix valgrind errors Moved cleanup of SCRBundleExtension members to make sure these objects are destroyed when an exception is thrown from DisableAndRemoveAllComponentManagers. Signed-off-by: The MathWorks, Inc. <jdicleme@mathworks.com> * Fix valgrind error Missed another error path where cleanup is necessary Signed-off-by: The MathWorks, Inc. <jdicleme@mathworks.com> Signed-off-by: The MathWorks, Inc. <jdicleme@mathworks.com> Co-authored-by: jdicleme <jdicleme@mathworks.com>
CppMicroServices · Dec 1, 2022 · c83f290 · c83f290
1 parent 0b4f93e
commit c83f290
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 20 deletions.
diff --git a/compendium/DeclarativeServices/src/SCRActivator.cpp b/compendium/DeclarativeServices/src/SCRActivator.cpp
@@ -152,7 +152,7 @@ void SCRActivator::CreateExtension(const cppmicroservices::Bundle& bundle)
     try {
       auto const& scrMap =
         ref_any_cast<cppmicroservices::AnyMap>(headers.at(SERVICE_COMPONENT));
-      auto ba = std::make_unique<SCRBundleExtension>(bundle.GetBundleContext(),
+      auto ba = std::make_unique<SCRBundleExtension>(bundle,
                                                      scrMap,
                                                      componentRegistry,
                                                      logger,

diff --git a/compendium/DeclarativeServices/src/SCRBundleExtension.cpp b/compendium/DeclarativeServices/src/SCRBundleExtension.cpp
@@ -42,19 +42,19 @@ using metadata::ComponentMetadata;
 using util::ObjectValidator;
 
 SCRBundleExtension::SCRBundleExtension(
-  const cppmicroservices::BundleContext& bundleContext,
+  const cppmicroservices::Bundle& bundle,
   const cppmicroservices::AnyMap& scrMetadata,
   const std::shared_ptr<ComponentRegistry>& registry,
   const std::shared_ptr<LogService>& logger,
   const std::shared_ptr<cppmicroservices::async::AsyncWorkService>&
     asyncWorkService,
   const std::shared_ptr<ConfigurationNotifier>& configNotifier)
-  : bundleContext(bundleContext)
+  : bundle_(bundle)
   , registry(registry)
   , logger(logger)
   , configNotifier(configNotifier)
 {
-  if (!bundleContext || !registry || !logger || scrMetadata.empty() ||
+  if (!bundle || !registry || !logger || scrMetadata.empty() ||
       !asyncWorkService || !configNotifier) {
     throw std::invalid_argument(
       "Invalid parameters passed to SCRBundleExtension constructor");
@@ -72,7 +72,7 @@ SCRBundleExtension::SCRBundleExtension(
       auto compManager =
         std::make_shared<ComponentManagerImpl>(oneCompMetadata,
                                                registry,
-                                               bundleContext,
+                                               bundle_.GetBundleContext(),
                                                logger,
                                                asyncWorkService,
                                                configNotifier,
@@ -85,30 +85,39 @@ SCRBundleExtension::SCRBundleExtension(
       throw;
     } catch (const cppmicroservices::SecurityException&) {
       DisableAndRemoveAllComponentManagers();
+      managers->clear();
       throw;
     } catch (const std::exception&) {
       logger->Log(cppmicroservices::logservice::SeverityLevel::LOG_ERROR,
                   "Failed to create ComponentManager with name " +
                     oneCompMetadata->name + " from bundle with Id " +
-                    std::to_string(bundleContext.GetBundle().GetBundleId()),
+                    std::to_string(bundle_.GetBundleId()),
                   std::current_exception());
     }
   }
   logger->Log(cppmicroservices::logservice::SeverityLevel::LOG_DEBUG,
               "Created instance of SCRBundleExtension for " +
-                bundleContext.GetBundle().GetSymbolicName());
+                bundle_.GetSymbolicName());
 }
 
 SCRBundleExtension::~SCRBundleExtension()
 {
-  DisableAndRemoveAllComponentManagers();
+  try {
+    DisableAndRemoveAllComponentManagers();
+  } catch(...) {
+    logger->Log(cppmicroservices::logservice::SeverityLevel::LOG_WARNING,
+                "Exception while removing component managers for bundle " + 
+                bundle_.GetSymbolicName(), std::current_exception());
+  }
+  managers->clear();
+  registry.reset();
 }
 
 void SCRBundleExtension::DisableAndRemoveAllComponentManagers()
 {
   logger->Log(cppmicroservices::logservice::SeverityLevel::LOG_DEBUG,
               "Deleting instance of SCRBundleExtension for " +
-                bundleContext.GetBundle().GetSymbolicName());
+                bundle_.GetSymbolicName());
   for (auto& compManager : *managers) {
     auto fut = compManager->Disable();
     registry->RemoveComponentManager(compManager);
@@ -123,8 +132,6 @@ void SCRBundleExtension::DisableAndRemoveAllComponentManagers()
                   std::current_exception());
     }
   }
-  managers->clear();
-  registry.reset();
 }
 } // scrimpl
 } // cppmicroservices
diff --git a/compendium/DeclarativeServices/src/SCRBundleExtension.hpp b/compendium/DeclarativeServices/src/SCRBundleExtension.hpp
@@ -51,7 +51,7 @@ class SCRBundleExtension
 {
 public:
   SCRBundleExtension(
-    const cppmicroservices::BundleContext& bundleContext,
+    const cppmicroservices::Bundle& bundle,
     const cppmicroservices::AnyMap& scrMetadata,
     const std::shared_ptr<ComponentRegistry>& registry,
     const std::shared_ptr<LogService>& logger,
@@ -70,7 +70,7 @@ class SCRBundleExtension
 
   void DisableAndRemoveAllComponentManagers();
 
-  cppmicroservices::BundleContext bundleContext;
+  cppmicroservices::Bundle bundle_;
   std::shared_ptr<ComponentRegistry> registry;
   std::shared_ptr<LogService> logger;
   std::shared_ptr<std::vector<std::shared_ptr<ComponentManager>>> managers;

diff --git a/compendium/DeclarativeServices/test/TestSCRBundleExtension.cpp b/compendium/DeclarativeServices/test/TestSCRBundleExtension.cpp
@@ -24,6 +24,7 @@
 #include "../src/SCRBundleExtension.hpp"
 #include "../src/metadata/Util.hpp"
 #include "Mocks.hpp"
+#include "TestUtils.hpp"
 #include "cppmicroservices/servicecomponent/ComponentConstants.hpp"
 #include <chrono>
 #include <cppmicroservices/BundleContext.h>
@@ -42,7 +43,7 @@ namespace cppmicroservices {
 namespace scrimpl {
 
 using cppmicroservices::AnyMap;
-// The fixture for testing class SCRActivator.
+// The fixture for testing class SCRBundleExtension.
 class SCRBundleExtensionTest : public ::testing::Test
 {
 protected:
@@ -81,7 +82,7 @@ TEST_F(SCRBundleExtensionTest, CtorInvalidArgs)
     bundleContext, fakeLogger, asyncWorkService);
   EXPECT_THROW(
     {
-      SCRBundleExtension bundleExt(BundleContext(),
+      SCRBundleExtension bundleExt(Bundle(),
                                    headers,
                                    mockRegistry,
                                    fakeLogger,
@@ -91,7 +92,7 @@ TEST_F(SCRBundleExtensionTest, CtorInvalidArgs)
     std::invalid_argument);
   EXPECT_THROW(
     {
-      SCRBundleExtension bundleExt(GetFramework().GetBundleContext(),
+      SCRBundleExtension bundleExt(GetFramework(),
                                    headers,
                                    nullptr,
                                    fakeLogger,
@@ -101,7 +102,7 @@ TEST_F(SCRBundleExtensionTest, CtorInvalidArgs)
     std::invalid_argument);
   EXPECT_THROW(
     {
-      SCRBundleExtension bundleExt(GetFramework().GetBundleContext(),
+      SCRBundleExtension bundleExt(GetFramework(),
                                    headers,
                                    mockRegistry,
                                    nullptr,
@@ -111,7 +112,7 @@ TEST_F(SCRBundleExtensionTest, CtorInvalidArgs)
     std::invalid_argument);
   EXPECT_THROW(
     {
-      SCRBundleExtension bundleExt(GetFramework().GetBundleContext(),
+      SCRBundleExtension bundleExt(GetFramework(),
                                    headers,
                                    mockRegistry,
                                    fakeLogger,
@@ -149,7 +150,7 @@ TEST_F(SCRBundleExtensionTest, CtorWithValidArgs)
   auto notifier = std::make_shared<ConfigurationNotifier>(
     GetFramework().GetBundleContext(), fakeLogger, asyncWorkService);
   EXPECT_NO_THROW({
-    SCRBundleExtension bundleExt(GetFramework().GetBundleContext(),
+    SCRBundleExtension bundleExt(GetFramework(),
                                  scr,
                                  mockRegistry,
                                  fakeLogger,
@@ -158,7 +159,7 @@ TEST_F(SCRBundleExtensionTest, CtorWithValidArgs)
     EXPECT_EQ(bundleExt.managers->size(), 0u);
   });
   EXPECT_NO_THROW({
-    SCRBundleExtension bundleExt(GetFramework().GetBundleContext(),
+    SCRBundleExtension bundleExt(GetFramework(),
                                  scr,
                                  mockRegistry,
                                  fakeLogger,
@@ -167,5 +168,38 @@ TEST_F(SCRBundleExtensionTest, CtorWithValidArgs)
     EXPECT_EQ(bundleExt.managers->size(), 1u);
   });
 }
+
+// Simulate a DS bundle is stopped before DS is able to cleanup the data associated
+// with the bundle.
+TEST_F(SCRBundleExtensionTest, DtorNoThrow) {
+    auto bundle = test::InstallAndStartBundle(GetFramework().GetBundleContext(), "TestBundleDSTOI1");
+      ASSERT_TRUE(static_cast<bool>(bundle));
+      auto fakeRegistry = std::make_shared<ComponentRegistry>();
+      auto fakeLogger = std::make_shared<FakeLogger>();
+      auto asyncWorkService =
+        std::make_shared<cppmicroservices::scrimpl::SCRAsyncWorkService>(
+          GetFramework().GetBundleContext(), fakeLogger);
+      auto notifier = std::make_shared<ConfigurationNotifier>(
+        GetFramework().GetBundleContext(), fakeLogger, asyncWorkService);
+      auto const& headers = ref_any_cast<cppmicroservices::AnyMap>(
+        bundle.GetHeaders().at("scr"));
+
+    EXPECT_NO_THROW(
+    {
+      SCRBundleExtension bundleExt(bundle,
+                                   headers,
+                                   fakeRegistry,
+                                   fakeLogger,
+                                   asyncWorkService,
+                                   notifier);
+      // stop the bundle prior to ~SCRBundleExtension being called. ~SCRBundleExtension
+      // attempts to access the Bundle object, so make sure accessing an invalid
+      // Bundle object doesn't throw.
+      bundle.Stop();
+    });
+
+    asyncWorkService->StopTracking();
+    fakeRegistry->Clear();
+}
 }
 }