configurations using the same pid are not updated properly (#754)

* configurations using the same pid are not updated properly

Fixed an issue whereby re-using a configuration pid did not cause the configuration to be sent to the ManagedService/ManagedServiceFactory correctly.

* Remove last change count instead of setting it to zero

Co-authored-by: jdicleme <jdicleme@mathworks.com>

compendium/ConfigurationAdmin/src/ConfigurationAdminImpl.cpp

@@ -624,7 +624,11 @@ std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationUpdated(
                                *(managedServiceWrapper->getTrackedService()),
                                properties,
                                *logger);
-          managedServiceWrapper->setLastUpdatedChangeCount(pid, changeCount);
+          if (removed) {
+            managedServiceWrapper->removeLastUpdatedChangeCount(pid);
+          } else {
+            managedServiceWrapper->setLastUpdatedChangeCount(pid, changeCount);
+          }
         }
       });
 
@@ -646,6 +650,7 @@ std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationUpdated(
               pid,
               *(managedServiceFactoryWrapper->getTrackedService()),
               *logger);
+            managedServiceFactoryWrapper->removeLastUpdatedChangeCount(pid);
           } else if (managedServiceFactoryWrapper->needsAnUpdateNotification(
                        pid, changeCount)) {
             notifyServiceUpdated(
@@ -663,7 +668,8 @@ std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationUpdated(
 
 std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationRemoved(
   const std::string& pid,
-  std::uintptr_t configurationId)
+  std::uintptr_t configurationId,
+  unsigned long changeCount)
 {
   std::promise<void> ready;
   std::shared_future<void> alreadyRemoved = ready.get_future();
@@ -691,7 +697,7 @@ std::shared_future<void> ConfigurationAdminImpl::NotifyConfigurationRemoved(
     RemoveFactoryInstanceIfRequired(pid);
   }
   if (configurationToInvalidate && hasBeenUpdated) {
-    auto removeFuture = NotifyConfigurationUpdated(pid, 0);
+    auto removeFuture = NotifyConfigurationUpdated(pid, changeCount);
     // This functor will run on another thread. Just being overly cautious to guarantee that the
     // ConfigurationImpl which has called this method doesn't run its own destructor.
     PerformAsync(

compendium/ConfigurationAdmin/src/ConfigurationAdminImpl.hpp

@@ -88,6 +88,11 @@ class TrackedServiceWrapper
     return lastUpdatedChangeCountPerPid[pid] < changeCount;
   }
 
+  void removeLastUpdatedChangeCount(const std::string& pid) noexcept {
+    std::unique_lock<std::mutex> lock(updatedChangeCountMutex);
+    (void)lastUpdatedChangeCountPerPid.erase(pid);
+  }
+
 private:
   std::string pid;
   std::shared_ptr<TrackedServiceType> trackedService;
@@ -194,7 +199,8 @@ class ConfigurationAdminImpl final
    */
   std::shared_future<void> NotifyConfigurationRemoved(
     const std::string& pid,
-    std::uintptr_t configurationId) override;
+    std::uintptr_t configurationId,
+    unsigned long changeCount) override;
 
   // methods from the cppmicroservices::ServiceTrackerCustomizer interface for ManagedService
   std::shared_ptr<

compendium/ConfigurationAdmin/src/ConfigurationAdminPrivate.hpp

@@ -105,7 +105,8 @@ class ConfigurationAdminPrivate
    */
   virtual std::shared_future<void> NotifyConfigurationRemoved(
     const std::string& pid,
-    std::uintptr_t configurationId) = 0;
+    std::uintptr_t configurationId,
+    unsigned long changeCount) = 0;
 };
 } // cmimpl
 } // cppmicroservices

compendium/ConfigurationAdmin/src/ConfigurationImpl.cpp

@@ -143,7 +143,7 @@ std::shared_future<void> ConfigurationImpl::Remove()
   std::lock_guard<std::mutex> lk{ configAdminMutex };
   if (configAdminImpl) {
     auto fut = configAdminImpl->NotifyConfigurationRemoved(
-      pid, reinterpret_cast<std::uintptr_t>(this));
+      pid, reinterpret_cast<std::uintptr_t>(this), changeCount);
     configAdminImpl = nullptr;
     return fut;
   }

compendium/ConfigurationAdmin/test/Mocks.hpp

@@ -123,8 +123,8 @@ class MockConfigurationAdminPrivate : public ConfigurationAdminPrivate
   MOCK_METHOD1(RemoveConfigurations, void(std::vector<ConfigurationAddedInfo>));
   MOCK_METHOD2(NotifyConfigurationUpdated,
                std::shared_future<void>(const std::string&, const unsigned long));
-  MOCK_METHOD2(NotifyConfigurationRemoved,
-               std::shared_future<void>(const std::string&, std::uintptr_t));
+  MOCK_METHOD3(NotifyConfigurationRemoved,
+               std::shared_future<void>(const std::string&, std::uintptr_t, unsigned long));
 };
 
 namespace async {

compendium/ConfigurationAdmin/test/TestConfigAdmin.cpp

@@ -326,7 +326,7 @@ TEST_F(ConfigAdminTests, testServiceUpdated)
   EXPECT_EQ(service->getCounter(), expectedCount);
 }
 
-TEST_F(ConfigAdminTests, testServiceRemoved)
+TEST_F(ConfigAdminTests, testConfigurationRemoved)
 {
   auto f = GetFramework();
   auto ctx = f.GetBundleContext();
@@ -342,17 +342,14 @@ TEST_F(ConfigAdminTests, testServiceRemoved)
   std::this_thread::sleep_for(DEFAULT_POLL_PERIOD);
   EXPECT_GE(service->getCounter(), 1);
 
-  auto const initConfiguredCount = service->getCounter();
-  int expectedCount = initConfiguredCount;
-
+  auto expectedCount = service->getCounter();
   auto configuration = m_configAdmin->GetConfiguration("cm.testservice");
 
   EXPECT_EQ(service->getCounter(), expectedCount);
 
   // Remove sends an asynchronous notification to the ManagedService so we
   // have to wait until it's finished before checking the result.
-  auto fut = configuration->Remove();
-  fut.get();
+  configuration->Remove().get();
 
   expectedCount -= 1;
   EXPECT_EQ(service->getCounter(), expectedCount);
@@ -528,10 +525,9 @@ TEST_F(ConfigAdminTests, testRemoveFactoryConfiguration)
   auto configuration_config1 =
     m_configAdmin->GetFactoryConfiguration("cm.testfactory", "config1");
 
-  auto fut = configuration_config1->Remove();
-  fut.get();
+  configuration_config1->Remove().get();
 
-  EXPECT_NE(serviceFactory->getRemovedCounter("cm.testfactory~config1"), 0);
+  EXPECT_EQ(serviceFactory->getRemovedCounter("cm.testfactory~config1"), 1);
   EXPECT_EQ(serviceFactory->getRemovedCounter("cm.testfactory~config2"), 0);
 
   // Should create a new configuration
@@ -543,6 +539,14 @@ TEST_F(ConfigAdminTests, testRemoveFactoryConfiguration)
             expectedCount_config1);
   EXPECT_EQ(serviceFactory->getUpdatedCounter("cm.testfactory~config2"),
             expectedCount_config2);
+
+  cppmicroservices::AnyMap props(
+    cppmicroservices::AnyMap::UNORDERED_MAP_CASEINSENSITIVE_KEYS);
+  props["anInt"] = 5;
+  configuration_config1->UpdateIfDifferent(props).second.get();
+  expectedCount_config1 = initConfiguredCount_config1;
+  EXPECT_EQ(serviceFactory->getUpdatedCounter("cm.testfactory~config1"), 7);
+
 }
 // This test confirms that if an object exists in the configuration repository
 // but has not yet been Updated prior to the start of the ManagedServiceFactory

compendium/ConfigurationAdmin/test/TestConfigurationImpl.cpp

@@ -69,7 +69,7 @@ TEST(TestConfigurationImpl, ThrowsWhenRemoved)
   ConfigurationImpl conf{ mockConfigAdmin.get(), pid, factoryPid, props };
   EXPECT_CALL(
     *mockConfigAdmin,
-    NotifyConfigurationRemoved(pid, reinterpret_cast<std::uintptr_t>(&conf)))
+    NotifyConfigurationRemoved(pid, reinterpret_cast<std::uintptr_t>(&conf), testing::_))
     .Times(1);
   EXPECT_NO_THROW(conf.Remove());
   EXPECT_THROW(conf.GetPid(), std::runtime_error);
@@ -95,7 +95,7 @@ TEST(TestConfigurationImpl, NoCallbacksAfterInvalidate)
   std::string factoryPid{ "test" };
   ConfigurationImpl conf{ mockConfigAdmin.get(), pid, factoryPid, props };
   EXPECT_CALL(*mockConfigAdmin,
-              NotifyConfigurationRemoved(testing::_, testing::_))
+              NotifyConfigurationRemoved(testing::_, testing::_, testing::_))
     .Times(0);
   EXPECT_CALL(*mockConfigAdmin, NotifyConfigurationUpdated(testing::_, testing::_))
     .Times(0);
@@ -164,7 +164,7 @@ TEST(TestConfigurationImpl, VerifyRemoveWithoutNotificationIfChangeCountEquals)
   std::string factoryPid{ "test" };
   ConfigurationImpl conf{ mockConfigAdmin.get(), pid, factoryPid, props };
   EXPECT_CALL(*mockConfigAdmin,
-              NotifyConfigurationRemoved(testing::_, testing::_))
+              NotifyConfigurationRemoved(testing::_, testing::_, testing::_))
     .Times(0);
   EXPECT_FALSE(conf.RemoveWithoutNotificationIfChangeCountEquals(0ul));
   EXPECT_TRUE(conf.RemoveWithoutNotificationIfChangeCountEquals(1ul));