Fix deadlock in ConfigurationAdminImpl::RemoveConfigurations (#745)

* Fix deadlock in ConfigurationAdminImpl::RemoveConfigurations

This is a fix for a deadlock bug caused by the WaitForAllAsync in ConfigurationAdmin::RemoveConfigurations.
The Use Case is as follows:
     A configuration object is defined in the manifest.json file.
      The User's main thread stopped the bundle which causes
         the ConfigurationAdminImp::RemoveConfigurations
         method to remove the configuration object from the
         ConfigurationAdmin repository and to send an Updated
         notification to the service instance.
         RemoveConfigurations would then execute a
         WaitForAllAsync to wait for all asynchronous threads to
         complete including the asynchronous thread that was
         launched as part of the Updated  notification.
      The user's Updated method also tried to stop the bundle.
      This means that it had to wait for the
      RemoveConfigurations method to complete.
      Each thread was waiting for the other to complete.
This fix removes the WaitForAllAsync and adds a test case to confirm the deadlock is gone. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Fix compiler error

Fix compiler error in Minimum Gcc build. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Change ConfigAdminTests.testManagedServiceRemoveConfigurationsDeadlock

Change ConfigAdminTests.testManagedServiceRemoveConfigurationsDeadlock so that it more closely matches the use case that caused the deadlock. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Update TestConfigAdmin.cpp

testManagedServiceRemoveConfigurationsDeadlock test. Changed Updated method to make sure that it only stops the bundle containing the cm.testdeadlock configuration object. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Update TestConfigAdmin.cpp

Added error checking for the Updated method used in testManagedServiceRemoveConfigurationsDeadlock. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

compendium/ConfigurationAdmin/src/ConfigurationAdminImpl.cpp

@@ -539,7 +539,6 @@ void ConfigurationAdminImpl::RemoveConfigurations(
     }
     ++idx;
   }
-  WaitForAllAsync();
 }
 
 std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationUpdated(

compendium/ConfigurationAdmin/test/TestConfigAdmin.cpp

@@ -78,11 +78,17 @@ void InstallAndStartDSAndConfigAdmin(::cppmicroservices::BundleContext& ctx)
   }
 }
 
+std::vector<cppmicroservices::Bundle> InstallBundles(cppmicroservices::BundleContext& ctx,
+        const std::string& bundleName)
+{
+  std::string path = PathToLib(bundleName);
+  return ctx.InstallBundles(path);
+}
+
 size_t installAndStartTestBundles(cppmicroservices::BundleContext& ctx,
                                   const std::string& bundleName)
 {
-  std::string path = PathToLib(bundleName);
-  auto bundles = ctx.InstallBundles(path);
+  auto bundles = InstallBundles(ctx, bundleName);
   for (auto& b : bundles) {
     b.Start();
   }
@@ -843,3 +849,103 @@ TEST_F(ConfigAdminTests, testMultipleManagedFactoriesInBundleGetUpdate)
                             1);
                 });
 }
+// <summary>
+/// Used by ConfigAdminTests.testManagedServiceRemoveConfigurationsDeadlock
+/// </summary>
+namespace cppmicroservices {
+namespace test {
+class TestManagedServiceInterface2
+  : public cppmicroservices::service::cm::ManagedService
+{
+public:
+  virtual ~TestManagedServiceInterface2() noexcept = default;
+  void Updated(const cppmicroservices::AnyMap&) override = 0; 
+};
+
+class TestManagedServiceImpl : public TestManagedServiceInterface2
+{
+public:
+  TestManagedServiceImpl(cppmicroservices::Framework& framework)
+    : m_framework(framework)
+   {}
+  virtual ~TestManagedServiceImpl() noexcept = default;
+
+  void Updated(const cppmicroservices::AnyMap& props) override
+  { 
+    // The Updated method is called for both Updated and Removed operations. 
+    // For the Removed operations, the properties are empty.
+    if (props.empty()) {
+      auto ctx = m_framework.GetBundleContext();
+      auto bundles =
+        ctx.GetBundles(PathToLib("TestBundleManagedServiceDeadlock"));
+      ASSERT_EQ(bundles.size(), 1ul);
+      for (auto& b : bundles) {
+        ASSERT_EQ(b.GetSymbolicName(), "TestBundleManagedServiceDeadlock");
+        b.Stop();
+      }
+    }
+  }
+
+private:
+  cppmicroservices::Framework m_framework;
+};
+}
+}
+/*
+* testManagedServiceRemoveConfigurationsDeadlock
+* 
+* This test was added in response to a deadlock bug. 
+* The Use Case is as follows:
+*     A configuration object is defined in the manifest.json file. (In
+*        this case it is defined in TestBundleManagedServiceDeadlock)
+*     The User's main thread stopped the bundle which causes 
+*         the ConfigurationAdminImpl RemoveConfigurations method 
+*         to remove the configuration object from the ConfigurationAdmin
+*         repository and to send a Removed notification to the service
+*         instance. RemoveConfigurations would then execute a WaitForAllAsync
+*         to wait for all asynchronous threads to complete including the 
+*         asynchronous thread that was launched as part of the Removed 
+*         notification. 
+*      The user's Updated method also tried to stop the bundle. This means
+*          that it had to wait for the RemoveConfigurations method to complete. 
+*      Hence the deadlock.
+*
+* The solution is to remove the WaitForAllAsync from RemoveConfigurations. 
+* This test will deadlock if the WaitForAllAsync function is not removed.
+* 
+*/
+TEST_F(ConfigAdminTests, testManagedServiceRemoveConfigurationsDeadlock)
+{
+  auto f = GetFramework();
+  auto ctx = f.GetBundleContext();
+
+  //Install and start the bundle containing the cm.testdeadlock configuration object.
+  auto bundles = InstallBundles(ctx, "TestBundleManagedServiceDeadlock");
+  ASSERT_FALSE(bundles.empty());
+  for (auto& b : bundles) {
+    b.Start();
+  }
+
+  // Register the service that implmenets the TestManagedServiceInterface2
+  // and ManagedService interface. The implementation class is TestManagedServiceImpl
+  // This service receives notifications when the cm.testdeadlock configuration object
+  // is updated or removed.
+  cppmicroservices::ServiceProperties serviceProperties;
+  serviceProperties["service.pid"] =
+  cppmicroservices::Any(std::string("cm.testdeadlock"));
+      (void)
+        ctx.RegisterService<cppmicroservices::test::TestManagedServiceInterface2,
+                            cppmicroservices::service::cm::ManagedService>(
+          std::make_shared<cppmicroservices::test::TestManagedServiceImpl>(f),
+          serviceProperties);
+
+  // Stop the bundle containing the cm.testdeadlock configuration object. This causes
+  // ConfigurationAdminImpl::RemoveConfigurations to execute. It sends an Updated notification
+  // to the TestManagedServiceImpl service. With the WaitForAllAsync method present in
+  // in RemoveConfigurations, the bundle Stop command won't return until the Updated 
+  // notification is complete. Unfortunately, the Updated method also tries to stop the 
+  // bundle and a deadlock results.
+  for (auto& b : bundles) {
+     b.Stop();
+  }
+}

compendium/test_bundles/CMakeLists.txt

@@ -59,6 +59,7 @@ add_subdirectory(DSFrenchDictionary)
 add_subdirectory(EnglishDictionary)
 
 add_subdirectory(ManagedServiceAndFactoryBundle)
+add_subdirectory(TestBundleManagedServiceDeadlock)
 add_subdirectory(TestBundleManagedServiceFactory)
 add_subdirectory(TestBundleNestedBundleStartManagedService)
 add_subdirectory(TestBundleMultipleManagedService)

compendium/test_bundles/TestBundleManagedServiceDeadlock/CMakeLists.txt

@@ -0,0 +1,6 @@
+usFunctionCreateDSTestBundle(TestBundleManagedServiceDeadlock)
+
+usFunctionCreateTestBundleWithResources(TestBundleManagedServiceDeadlock
+  RESOURCES manifest.json
+  BUNDLE_SYMBOLIC_NAME TestBundleManagedServiceDeadlock
+  OTHER_LIBRARIES usTestInterfaces usServiceComponent cm)

compendium/test_bundles/TestBundleManagedServiceDeadlock/resources/manifest.json

@@ -0,0 +1,16 @@
+{
+  "bundle.symbolic_name": "TestBundleManagedServiceDeadlock",
+  "bundle.name": "TestBundleManagedServiceDeadlock",
+  "bundle.activator": false,
+   "cm": {
+    "version": 1,
+    "configurations": [
+      {
+        "pid": "cm.testdeadlock",
+        "properties": {
+          "anInt": 2
+        }
+      } 
+    ]
+  }
+}