Unify depependency cycle check code.

This commit removes a distinction in how dependency objects are checked for
cycles in the resulting graph depending on whether they are part of the
initially loaded configuration during process startup or as part of a runtime
update.

The DependencyCycleChecker helper class is extended with a mechanism that
allows additional dependencies to be considered during the cycle search. This
allows using it to check for cycles before actually registering the
dependencies with the checkables.

The aforementioned case-distinction for initial/runtime-update config is
removed by making use of the newly added BeforeOnAllConfigLoaded signal to
perform the cycle check at once for each batch of dependencies inside
ConfigItem::CommitNewItems() for both cases now. During the initial config
loading, there can be multiple batches of dependencies as objects from apply
rules are created separately, so parts of the dependency graph might be visited
multiple times now, however that is limited to a minimum as only parts of the
graph that are reachable from the newly added dependencies are searched.

lib/cli/daemonutility.cpp

@@ -256,17 +256,6 @@ bool DaemonUtility::LoadConfigFiles(const std::vector<std::string>& configs,
 	upq.SetName("DaemonUtility::LoadConfigFiles");
 	bool result = ConfigItem::CommitItems(ascope.GetContext(), upq, newItems);
 
-	if (result) {
-		try {
-			Dependency::AssertNoCycles();
-		} catch (...) {
-			Log(LogCritical, "config")
-				<< DiagnosticInformation(boost::current_exception(), false);
-
-			result = false;
-		}
-	}
-
 	if (!result) {
 		ConfigCompilerContext::GetInstance()->CancelObjectsFile();
 		return false;

lib/icinga/dependency.cpp

@@ -17,7 +17,12 @@ using namespace icinga;
 
 REGISTER_TYPE(Dependency);
 
-bool Dependency::m_AssertNoCyclesForIndividualDeps = false;
+INITIALIZE_ONCE(&Dependency::StaticInitialize);
+
+void Dependency::StaticInitialize()
+{
+	ConfigType::Get<Dependency>()->BeforeOnAllConfigLoaded.connect(&BeforeOnAllConfigLoadedHandler);
+}
 
 /**
  * Helper class to search for cycles in the dependency graph.
@@ -31,6 +36,7 @@ class DependencyCycleChecker
 	{
 		bool Visited = false;
 		bool OnStack = false;
+		std::vector<Dependency::Ptr> ExtraDependencies;
 	};
 
 	std::unordered_map<Checkable::Ptr, Node> m_Nodes;
@@ -40,6 +46,19 @@ class DependencyCycleChecker
 	std::vector<std::variant<Dependency::Ptr, Service::Ptr>> m_Stack;
 
 public:
+	/**
+	 * Add a dependency to this DependencyCycleChecker that will be considered by AssertNoCycle() in addition to
+	 * dependencies already registered to the checkables. This allows checking if additional dependencies would cause
+	 * a cycle before actually registering them to the checkables.
+	 *
+	 * @param dependency Dependency to additionally consider during the cycle search.
+	 */
+	void AddExtraDependency(Dependency::Ptr dependency)
+	{
+		auto& node = m_Nodes[dependency->GetChild()];
+		node.ExtraDependencies.emplace_back(std::move(dependency));
+	}
+
 	/**
 	 * Searches the dependency graph for cycles and throws an exception if one is found.
 	 *
@@ -110,23 +129,43 @@ public:
 			m_Stack.pop_back();
 		}
 
+		// Additional dependencies to consider
+		for (const auto& dep : node.ExtraDependencies) {
+			m_Stack.emplace_back(dep);
+			AssertNoCycle(dep->GetParent());
+			m_Stack.pop_back();
+		}
+
 		node.OnStack = false;
 	}
 };
 
-void Dependency::AssertNoCycles()
+/**
+ * Checks that adding these new dependencies to the configuration does not introduce any cycles.
+ *
+ * This is done as an optimization: cycles are checked once for all dependencies in a batch of config objects instead
+ * of individually per dependency in Dependency::OnAllConfigLoaded(). For runtime updates, this function may still be
+ * called for single objects.
+ *
+ * @param items Config items containing Dependency objects added to the running configuration.
+ */
+void Dependency::BeforeOnAllConfigLoadedHandler(const ConfigItems& items)
 {
 	DependencyCycleChecker checker;
 
-	for (auto& host : ConfigType::GetObjectsByType<Host>()) {
-		checker.AssertNoCycle(host);
-	}
+	// Resolve parent/child names to Checkable::Ptr and temporarily add the edges to the checker.
+	// The dependencies are later registered to the checkables by Dependency::OnAllConfigLoaded().
+	items.ForEachObject<Dependency>([&checker](Dependency::Ptr dependency) {
+		dependency->InitChildParentReferences();
+		checker.AddExtraDependency(std::move(dependency));
+	});
 
-	for (auto& service : ConfigType::GetObjectsByType<Service>()) {
-		checker.AssertNoCycle(service);
-	}
-
-	m_AssertNoCyclesForIndividualDeps = true;
+	// It's sufficient to search for cycles starting from newly added dependencies only: if a newly added dependency is
+	// part of a cycle, that cycle is reachable from both the child and the parent of that dependency. The cycle search
+	// is started from the parent as a slight optimization as that will traverse fewer edges if there is no cycle.
+	items.ForEachObject<Dependency>([&checker](const Dependency::Ptr& dependency) {
+		checker.AssertNoCycle(dependency->GetParent());
+	});
 }
 
 String DependencyNameComposer::MakeName(const String& shortName, const Object::Ptr& context) const
@@ -178,10 +217,8 @@ void Dependency::OnConfigLoaded()
 	SetStateFilter(FilterArrayToInt(GetStates(), Notification::GetStateFilterMap(), defaultFilter));
 }
 
-void Dependency::OnAllConfigLoaded()
+void Dependency::InitChildParentReferences()
 {
-	ObjectImpl<Dependency>::OnAllConfigLoaded();
-
 	Host::Ptr childHost = Host::GetByName(GetChildHostName());
 
 	if (childHost) {
@@ -205,21 +242,17 @@ void Dependency::OnAllConfigLoaded()
 
 	if (!m_Parent)
 		BOOST_THROW_EXCEPTION(ScriptError("Dependency '" + GetName() + "' references a parent host/service which doesn't exist.", GetDebugInfo()));
+}
+
+void Dependency::OnAllConfigLoaded()
+{
+	ObjectImpl<Dependency>::OnAllConfigLoaded();
+
+	// InitChildParentReferences() has to be called before.
+	VERIFY(m_Child && m_Parent);
 
 	m_Child->AddDependency(this);
 	m_Parent->AddReverseDependency(this);
-
-	if (m_AssertNoCyclesForIndividualDeps) {
-		DependencyCycleChecker checker;
-
-		try {
-			checker.AssertNoCycle(m_Parent);
-		} catch (...) {
-			m_Child->RemoveDependency(this);
-			m_Parent->RemoveReverseDependency(this);
-			throw;
-		}
-	}
 }
 
 void Dependency::Stop(bool runtimeRemoved)

lib/icinga/dependency.hpp

@@ -5,6 +5,7 @@
 
 #include "icinga/i2-icinga.hpp"
 #include "icinga/dependency-ti.hpp"
+#include "config/configitem.hpp"
 
 namespace icinga
 {
@@ -22,6 +23,8 @@ class Service;
 class Dependency final : public ObjectImpl<Dependency>
 {
 public:
+	static void StaticInitialize();
+
 	DECLARE_OBJECT(Dependency);
 	DECLARE_OBJECTNAME(Dependency);
 
@@ -36,9 +39,8 @@ public:
 
 	static void EvaluateApplyRules(const intrusive_ptr<Host>& host);
 	static void EvaluateApplyRules(const intrusive_ptr<Service>& service);
-	static void AssertNoCycles();
 
-	/* Note: Only use them for unit test mocks. Prefer OnConfigLoaded(). */
+	/* Note: Only use them for unit test mocks. Prefer InitChildParentReferences(). */
 	void SetParent(intrusive_ptr<Checkable> parent);
 	void SetChild(intrusive_ptr<Checkable> child);
 
@@ -46,15 +48,16 @@ protected:
 	void OnConfigLoaded() override;
 	void OnAllConfigLoaded() override;
 	void Stop(bool runtimeRemoved) override;
+	void InitChildParentReferences();
 
 private:
 	Checkable::Ptr m_Parent;
 	Checkable::Ptr m_Child;
 
-	static bool m_AssertNoCyclesForIndividualDeps;
-
 	static bool EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule, bool skipFilter);
 	static bool EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule, bool skipFilter = false);
+
+	static void BeforeOnAllConfigLoadedHandler(const ConfigItems& items);
 };
 
 }