DependencyGroup: don't change the keys of m_Members after construction

This prevents the use of DependencyGroup for storing the dependencies during
the early registration (m_DependencyGroupsPushedToRegistry = false),
m_PendingDependencies is introduced as a replacement to store the dependencies
at that time.

lib/icinga/checkable-dependency.cpp

@@ -26,15 +26,12 @@ static constexpr int l_MaxDependencyRecursionLevel(256);
 void Checkable::PushDependencyGroupsToRegistry()
 {
 	std::lock_guard lock(m_DependencyMutex);
-	if (!m_DependencyGroupsPushedToRegistry) {
-		m_DependencyGroupsPushedToRegistry = true;
-
-		decltype(m_DependencyGroups) dependencyGroups;
-		m_DependencyGroups.swap(dependencyGroups);
-
-		for (auto& [dependencyGroupKey, dependencyGroup] : dependencyGroups) {
-			m_DependencyGroups.emplace(dependencyGroupKey, DependencyGroup::Register(dependencyGroup));
+	if (m_PendingDependencies != nullptr) {
+		for (const auto& [key, dependencies] : *m_PendingDependencies) {
+			String redundancyGroup = std::holds_alternative<String>(key) ? std::get<String>(key) : "";
+			m_DependencyGroups.emplace(key, DependencyGroup::Register(new DependencyGroup(redundancyGroup, dependencies)));
 		}
+		m_PendingDependencies.reset();
 	}
 }
 
@@ -78,12 +75,8 @@ void Checkable::AddDependency(const Dependency::Ptr& dependency)
 	std::unique_lock lock(m_DependencyMutex);
 
 	auto dependencyGroupKey(GetDependencyGroupKey(dependency));
-	if (!m_DependencyGroupsPushedToRegistry) {
-		auto& dependencyGroup = m_DependencyGroups[dependencyGroupKey];
-		if (!dependencyGroup) {
-			dependencyGroup = new DependencyGroup(dependency->GetRedundancyGroup());
-		}
-		dependencyGroup->AddDependency(dependency);
+	if (m_PendingDependencies != nullptr) {
+		(*m_PendingDependencies)[dependencyGroupKey].emplace(dependency);
 		return;
 	}
 
@@ -151,10 +144,17 @@ void Checkable::RemoveDependency(const Dependency::Ptr& dependency, bool runtime
 	}
 }
 
-std::vector<Dependency::Ptr> Checkable::GetDependencies() const
+std::vector<Dependency::Ptr> Checkable::GetDependencies(bool includePending) const
 {
 	std::unique_lock<std::mutex> lock(m_DependencyMutex);
 	std::vector<Dependency::Ptr> dependencies;
+
+	if (includePending && m_PendingDependencies != nullptr) {
+		for (const auto& [group, groupDeps] : *m_PendingDependencies) {
+			dependencies.insert(dependencies.end(), groupDeps.begin(), groupDeps.end());
+		}
+	}
+
 	for (const auto& [_, dependencyGroup] : m_DependencyGroups) {
 		auto tmpDependencies(dependencyGroup->GetDependenciesForChild(this));
 		dependencies.insert(dependencies.end(), tmpDependencies.begin(), tmpDependencies.end());

lib/icinga/checkable.hpp

@@ -190,7 +190,7 @@ public:
 	std::vector<intrusive_ptr<DependencyGroup>> GetDependencyGroups() const;
 	void AddDependency(const intrusive_ptr<Dependency>& dependency);
 	void RemoveDependency(const intrusive_ptr<Dependency>& dependency, bool runtimeRemoved = false);
-	std::vector<intrusive_ptr<Dependency> > GetDependencies() const;
+	std::vector<intrusive_ptr<Dependency> > GetDependencies(bool includePending = false) const;
 	bool HasAnyDependencies() const;
 
 	void AddReverseDependency(const intrusive_ptr<Dependency>& dep);
@@ -251,9 +251,19 @@ private:
 
 	/* Dependencies */
 	mutable std::mutex m_DependencyMutex;
-	bool m_DependencyGroupsPushedToRegistry{false};
 	std::map<std::variant<Checkable*, String>, intrusive_ptr<DependencyGroup>> m_DependencyGroups;
 	std::set<intrusive_ptr<Dependency> > m_ReverseDependencies;
+	/**
+	 * Registering a checkable to its parent DependencyGroups is delayed during config loading until all dependencies
+	 * were registered on the checkable. m_PendingDependencies is used to temporarily store the dependencies until then.
+	 * It is a pointer type for two reasons:
+	 * 1. The field is no longer needed after the DependencyGroups were registered, having it as a pointer reduces the
+	 *    overhead from sizeof(std::map<>) to sizeof(std::map<>*).
+	 * 2. It allows the field to also be used as a flag: the delayed group registration is only done until it is reset
+	 *    to nullptr.
+	 */
+	std::unique_ptr<std::map<std::variant<Checkable*, String>, std::set<intrusive_ptr<Dependency>>>>
+		m_PendingDependencies {std::make_unique<decltype(m_PendingDependencies)::element_type>()};
 
 	void GetAllChildrenInternal(std::set<Checkable::Ptr>& seenChildren, int level = 0) const;
 

lib/icinga/dependency-group.cpp

@@ -46,17 +46,18 @@ std::pair<std::set<Dependency::Ptr>, bool> DependencyGroup::Unregister(const Dep
 {
 	std::lock_guard lock(m_RegistryMutex);
 	if (auto it(m_Registry.find(dependencyGroup)); it != m_Registry.end()) {
-		auto existingGroup(*it);
+		auto& existingGroup(*it);
 		auto dependencies(existingGroup->GetDependenciesForChild(child.get()));
 
 		for (const auto& dependency : dependencies) {
 			existingGroup->RemoveDependency(dependency);
 		}
 
-		if (existingGroup->IsEmpty()) {
+		bool remove = !existingGroup->HasChildren();
+		if (remove) {
 			m_Registry.erase(it);
 		}
-		return {{dependencies.begin(), dependencies.end()}, existingGroup->IsEmpty()};
+		return {{dependencies.begin(), dependencies.end()}, remove};
 	}
 	return {{}, false};
 }
@@ -72,14 +73,11 @@ size_t DependencyGroup::GetRegistrySize()
 	return m_Registry.size();
 }
 
-DependencyGroup::DependencyGroup(String name): m_RedundancyGroupName(std::move(name))
-{
-}
-
-DependencyGroup::DependencyGroup(String name, const std::set<Dependency::Ptr>& dependencies): m_RedundancyGroupName(std::move(name))
+DependencyGroup::DependencyGroup(String name, const std::set<Dependency::Ptr>& dependencies)
+	: m_RedundancyGroupName(std::move(name))
 {
 	for (const auto& dependency : dependencies) {
-		AddDependency(dependency);
+		m_Members[MakeCompositeKeyFor(dependency)].emplace(dependency->GetChild().get(), dependency.get());
 	}
 }
 
@@ -103,14 +101,14 @@ DependencyGroup::CompositeKeyType DependencyGroup::MakeCompositeKeyFor(const Dep
 }
 
 /**
- * Check if the current dependency group is empty.
+ * Check if the current dependency has any children.
  *
- * @return bool - Returns true if the current dependency group has no members, otherwise false.
+ * @return bool - Returns true if the current dependency group has children, otherwise false.
  */
-bool DependencyGroup::IsEmpty() const
+bool DependencyGroup::HasChildren() const
 {
 	std::lock_guard lock(m_Mutex);
-	return m_Members.empty();
+	return std::any_of(m_Members.begin(), m_Members.end(), [](const auto& pair) { return !pair.second.empty(); });
 }
 
 /**
@@ -142,7 +140,6 @@ void DependencyGroup::LoadParents(std::set<Checkable::Ptr>& parents) const
 {
 	std::lock_guard lock(m_Mutex);
 	for (auto& [compositeKey, children] : m_Members) {
-		ASSERT(!children.empty()); // We should never have an empty map for any given key at any given time.
 		parents.insert(std::get<0>(compositeKey));
 	}
 }
@@ -174,11 +171,12 @@ void DependencyGroup::AddDependency(const Dependency::Ptr& dependency)
 {
 	std::lock_guard lock(m_Mutex);
 	auto compositeKey(MakeCompositeKeyFor(dependency));
-	if (auto it(m_Members.find(compositeKey)); it != m_Members.end()) {
-		it->second.emplace(dependency->GetChild().get(), dependency.get());
-	} else {
-		m_Members.emplace(compositeKey, MemberValueType{{dependency->GetChild().get(), dependency.get()}});
-	}
+	auto it = m_Members.find(compositeKey);
+
+	// The dependency must be compatible with the group, i.e. its parent config must be known in the group already.
+	VERIFY(it != m_Members.end());
+
+	it->second.emplace(dependency->GetChild().get(), dependency.get());
 }
 
 /**
@@ -196,10 +194,6 @@ void DependencyGroup::RemoveDependency(const Dependency::Ptr& dependency)
 				// This will also remove the child Checkable from the multimap container
 				// entirely if this was the last child of it.
 				it->second.erase(childrenIt);
-				// If the composite key has no more children left, we can remove it entirely as well.
-				if (it->second.empty()) {
-					m_Members.erase(it);
-				}
 				return;
 			}
 		}

lib/icinga/dependency.cpp

@@ -123,7 +123,7 @@ public:
 		}
 
 		// Explicitly configured dependency objects
-		for (const auto& dep : checkable->GetDependencies()) {
+		for (const auto& dep : checkable->GetDependencies(/* includePending = */ true)) {
 			m_Stack.emplace_back(dep);
 			AssertNoCycle(dep->GetParent());
 			m_Stack.pop_back();

lib/icinga/dependency.hpp

@@ -132,7 +132,6 @@ public:
 	using MemberValueType = std::unordered_multimap<const Checkable*, Dependency*>;
 	using MembersMap = std::map<CompositeKeyType, MemberValueType>;
 
-	explicit DependencyGroup(String name);
 	DependencyGroup(String name, const std::set<Dependency::Ptr>& dependencies);
 
 	static DependencyGroup::Ptr Register(const DependencyGroup::Ptr& dependencyGroup);
@@ -151,7 +150,7 @@ public:
 		return !m_RedundancyGroupName.IsEmpty();
 	}
 
-	bool IsEmpty() const;
+	bool HasChildren() const;
 	void AddDependency(const Dependency::Ptr& dependency);
 	void RemoveDependency(const Dependency::Ptr& dependency);
 	std::vector<Dependency::Ptr> GetDependenciesForChild(const Checkable* child) const;