diff --git a/lib/base/type.cpp b/lib/base/type.cpp index 1dcef39bfc9..47829e88705 100644 --- a/lib/base/type.cpp +++ b/lib/base/type.cpp @@ -72,6 +72,43 @@ std::vector Type::GetAllTypes() return types; } +std::vector Type::SortByLoadDependencies(const std::vector& types) +{ + std::vector sorted; + std::unordered_set pending, loaded; + bool ready; + + sorted.reserve(types.size()); + pending.reserve(types.size()); + loaded.reserve(types.size()); + + for (auto& type : types) { + pending.emplace(type.get()); + } + + while (!pending.empty()) { + auto type (*pending.begin()); + + do { + ready = true; + + for (auto dep : type->GetLoadDependencies()) { + if (loaded.find(dep) == loaded.end() && pending.find(dep) != pending.end()) { + type = dep; + ready = false; + break; + } + } + } while (!ready); + + sorted.emplace_back(type); + loaded.emplace(type); + pending.erase(type); + } + + return sorted; +} + String Type::GetPluralName() const { String name = GetName(); diff --git a/lib/base/type.hpp b/lib/base/type.hpp index 7b8d1cacb64..580cb2441c3 100644 --- a/lib/base/type.hpp +++ b/lib/base/type.hpp @@ -82,6 +82,7 @@ class Type : public Object static void Register(const Type::Ptr& type); static Type::Ptr GetByName(const String& name); static std::vector GetAllTypes(); + static std::vector SortByLoadDependencies(const std::vector& types); void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty) override; Value GetField(int id) const override; diff --git a/lib/config/configitem.cpp b/lib/config/configitem.cpp index 9dc0f1aa25d..b8dea9c20ce 100644 --- a/lib/config/configitem.cpp +++ b/lib/config/configitem.cpp @@ -450,74 +450,55 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue << "Committing " << total << " new items."; #endif /* I2_DEBUG */ - std::set types; - std::set completed_types; + std::vector types; int itemsCount {0}; for (const Type::Ptr& type : Type::GetAllTypes()) { if (ConfigObject::TypeInstance->IsAssignableFrom(type)) - types.insert(type); + types.emplace_back(type); } - while (types.size() != completed_types.size()) { - for (const Type::Ptr& type : types) { - if (completed_types.find(type) != completed_types.end()) - continue; - - bool unresolved_dep = false; - - /* skip this type (for now) if there are unresolved load dependencies */ - for (auto pLoadDep : type->GetLoadDependencies()) { - if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) { - unresolved_dep = true; - break; - } - } + types = Type::SortByLoadDependencies(types); - if (unresolved_dep) - continue; - - std::atomic committed_items(0); - std::mutex newItemsMutex; + for (auto& type : types) { + std::atomic committed_items(0); + std::mutex newItemsMutex; - { - auto items (itemsByType.find(type.get())); + { + auto items (itemsByType.find(type.get())); - if (items != itemsByType.end()) { - upq.ParallelFor(items->second, [&committed_items, &newItems, &newItemsMutex](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; + if (items != itemsByType.end()) { + upq.ParallelFor(items->second, [&committed_items, &newItems, &newItemsMutex](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - if (!item->Commit(ip.second)) { - if (item->IsIgnoreOnError()) { - item->Unregister(); - } - - return; + if (!item->Commit(ip.second)) { + if (item->IsIgnoreOnError()) { + item->Unregister(); } - committed_items++; + return; + } - std::unique_lock lock(newItemsMutex); - newItems.emplace_back(item); - }); + committed_items++; - upq.Join(); - } - } + std::unique_lock lock(newItemsMutex); + newItems.emplace_back(item); + }); - itemsCount += committed_items; + upq.Join(); + } + } - completed_types.insert(type); + itemsCount += committed_items; #ifdef I2_DEBUG - if (committed_items > 0) - Log(LogDebug, "configitem") - << "Committed " << committed_items << " items of type '" << type->GetName() << "'."; + if (committed_items > 0) + Log(LogDebug, "configitem") + << "Committed " << committed_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; - } + if (upq.HasExceptions()) + return false; } #ifdef I2_DEBUG @@ -525,105 +506,83 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue << "Committed " << itemsCount << " items."; #endif /* I2_DEBUG */ - completed_types.clear(); - - while (types.size() != completed_types.size()) { - for (const Type::Ptr& type : types) { - if (completed_types.find(type) != completed_types.end()) - continue; - - bool unresolved_dep = false; - - /* skip this type (for now) if there are unresolved load dependencies */ - for (auto pLoadDep : type->GetLoadDependencies()) { - if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) { - unresolved_dep = true; - break; - } - } - - if (unresolved_dep) - continue; - - std::atomic notified_items(0); + for (const Type::Ptr& type : types) { + std::atomic notified_items(0); - { - auto items (itemsByType.find(type.get())); + { + auto items (itemsByType.find(type.get())); - if (items != itemsByType.end()) { - upq.ParallelFor(items->second, [¬ified_items](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; + if (items != itemsByType.end()) { + upq.ParallelFor(items->second, [¬ified_items](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - if (!item->m_Object) - return; + if (!item->m_Object) + return; - try { - item->m_Object->OnAllConfigLoaded(); - notified_items++; - } catch (const std::exception& ex) { - if (!item->m_IgnoreOnError) - throw; + try { + item->m_Object->OnAllConfigLoaded(); + notified_items++; + } catch (const std::exception& ex) { + if (!item->m_IgnoreOnError) + throw; - Log(LogNotice, "ConfigObject") - << "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex); + Log(LogNotice, "ConfigObject") + << "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex); - item->Unregister(); + item->Unregister(); - { - std::unique_lock lock(item->m_Mutex); - item->m_IgnoredItems.push_back(item->m_DebugInfo.Path); - } + { + std::unique_lock lock(item->m_Mutex); + item->m_IgnoredItems.push_back(item->m_DebugInfo.Path); } - }); + } + }); - upq.Join(); - } + upq.Join(); } - - completed_types.insert(type); + } #ifdef I2_DEBUG - if (notified_items > 0) - Log(LogDebug, "configitem") - << "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'."; + if (notified_items > 0) + Log(LogDebug, "configitem") + << "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; + if (upq.HasExceptions()) + return false; - notified_items = 0; - for (auto loadDep : type->GetLoadDependencies()) { - auto items (itemsByType.find(loadDep)); + notified_items = 0; + for (auto loadDep : type->GetLoadDependencies()) { + auto items (itemsByType.find(loadDep)); - if (items != itemsByType.end()) { - upq.ParallelFor(items->second, [&type, ¬ified_items](const ItemPair& ip) { - const ConfigItem::Ptr& item = ip.first; + if (items != itemsByType.end()) { + upq.ParallelFor(items->second, [&type, ¬ified_items](const ItemPair& ip) { + const ConfigItem::Ptr& item = ip.first; - if (!item->m_Object) - return; + if (!item->m_Object) + return; - ActivationScope ascope(item->m_ActivationContext); - item->m_Object->CreateChildObjects(type); - notified_items++; - }); - } + ActivationScope ascope(item->m_ActivationContext); + item->m_Object->CreateChildObjects(type); + notified_items++; + }); } + } - upq.Join(); + upq.Join(); #ifdef I2_DEBUG - if (notified_items > 0) - Log(LogDebug, "configitem") - << "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'."; + if (notified_items > 0) + Log(LogDebug, "configitem") + << "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'."; #endif /* I2_DEBUG */ - if (upq.HasExceptions()) - return false; + if (upq.HasExceptions()) + return false; - // Make sure to activate any additionally generated items - if (!CommitNewItems(context, upq, newItems)) - return false; - } + // Make sure to activate any additionally generated items + if (!CommitNewItems(context, upq, newItems)) + return false; } return true; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 1c972d6d933..03625144ffe 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -121,6 +121,25 @@ add_boost_test(base base_type/assign base_type/byname base_type/instantiate + base_type/sortbyloaddependencies0 + base_type/sortbyloaddependencies1 + base_type/sortbyloaddependencies2related + base_type/sortbyloaddependencies2unrelated + base_type/sortbyloaddependencies3related + base_type/sortbyloaddependencies3unrelated + base_type/sortbyloaddependencies4related + base_type/sortbyloaddependencies4unrelated + base_type/sortbyloaddependencies5related + base_type/sortbyloaddependencies5unrelated + base_type/sortbyloaddependencies1_withforeign + base_type/sortbyloaddependencies2related_withforeign + base_type/sortbyloaddependencies2unrelated_withforeign + base_type/sortbyloaddependencies3related_withforeign + base_type/sortbyloaddependencies3unrelated_withforeign + base_type/sortbyloaddependencies4related_withforeign + base_type/sortbyloaddependencies4unrelated_withforeign + base_type/sortbyloaddependencies5related_withforeign + base_type/sortbyloaddependencies5unrelated_withforeign base_utility/parse_version base_utility/compare_version base_utility/comparepasswords_works diff --git a/test/base-type.cpp b/test/base-type.cpp index 21bcf439d35..0d2aec3a0fe 100644 --- a/test/base-type.cpp +++ b/test/base-type.cpp @@ -6,9 +6,110 @@ #include "base/application.hpp" #include "base/type.hpp" #include +#include +#include using namespace icinga; +class TestType : public Type +{ +public: + TestType(std::unordered_set loadDependencies = {}) : m_LoadDependencies(std::move(loadDependencies)) + { + } + + const std::unordered_set& GetLoadDependencies() const override + { + return m_LoadDependencies; + } + + String GetName() const override + { + throw std::runtime_error("not implemented"); + } + + Type::Ptr GetBaseType() const override + { + throw std::runtime_error("not implemented"); + } + + int GetAttributes() const override + { + throw std::runtime_error("not implemented"); + } + + int GetFieldId(const String&) const override + { + throw std::runtime_error("not implemented"); + } + + Field GetFieldInfo(int) const override + { + throw std::runtime_error("not implemented"); + } + + int GetFieldCount() const override + { + throw std::runtime_error("not implemented"); + } + +protected: + ObjectFactory GetFactory() const override + { + throw std::runtime_error("not implemented"); + } + +private: + std::unordered_set m_LoadDependencies; +}; + +static void TestSortByLoadDependenciesUnrelated(int amount, bool withDepsToForeign = false) +{ + std::vector expected; + + for (int i = 0; i < amount; ++i) { + expected.emplace_back(withDepsToForeign ? new TestType({new TestType()}) : new TestType()); + } + + std::sort(expected.begin(), expected.end()); + + auto input (expected); + + do { + auto actual (Type::SortByLoadDependencies(input)); + + std::sort(actual.begin(), actual.end()); + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), expected.begin(), expected.end()); + } while (std::next_permutation(input.begin(), input.end())); +} + +static void TestSortByLoadDependenciesRelated(int amount, bool withDepsToForeign = false) +{ + std::vector expected; + + for (int i = 0; i < amount; ++i) { + if (i) { + expected.emplace_back( + withDepsToForeign + ? new TestType({expected.at(expected.size() - 1u).get(), new TestType()}) + : new TestType({expected.at(expected.size() - 1u).get()}) + ); + } else { + expected.emplace_back(withDepsToForeign ? new TestType({new TestType()}) : new TestType()); + } + } + + auto input (expected); + + std::sort(input.begin(), input.end()); + + do { + auto actual (Type::SortByLoadDependencies(input)); + + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), expected.begin(), expected.end()); + } while (std::next_permutation(input.begin(), input.end())); +} + BOOST_AUTO_TEST_SUITE(base_type) BOOST_AUTO_TEST_CASE(gettype) @@ -44,4 +145,108 @@ BOOST_AUTO_TEST_CASE(instantiate) BOOST_CHECK(p); } +BOOST_AUTO_TEST_CASE(sortbyloaddependencies0) +{ + std::vector empty; + auto actual (Type::SortByLoadDependencies(empty)); + + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), empty.begin(), empty.end()); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies1) +{ + std::vector one ({new TestType()}); + auto actual (Type::SortByLoadDependencies(one)); + + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), one.begin(), one.end()); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies2unrelated) +{ + TestSortByLoadDependenciesUnrelated(2); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies2related) +{ + TestSortByLoadDependenciesRelated(2); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies3unrelated) +{ + TestSortByLoadDependenciesUnrelated(3); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies3related) +{ + TestSortByLoadDependenciesRelated(3); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies4unrelated) +{ + TestSortByLoadDependenciesUnrelated(4); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies4related) +{ + TestSortByLoadDependenciesRelated(4); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies5unrelated) +{ + TestSortByLoadDependenciesUnrelated(5); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies5related) +{ + TestSortByLoadDependenciesRelated(5); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies1_withforeign) +{ + std::vector one ({new TestType({new TestType()})}); + auto actual (Type::SortByLoadDependencies(one)); + + BOOST_CHECK_EQUAL_COLLECTIONS(actual.begin(), actual.end(), one.begin(), one.end()); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies2unrelated_withforeign) +{ + TestSortByLoadDependenciesUnrelated(2, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies2related_withforeign) +{ + TestSortByLoadDependenciesRelated(2, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies3unrelated_withforeign) +{ + TestSortByLoadDependenciesUnrelated(3, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies3related_withforeign) +{ + TestSortByLoadDependenciesRelated(3, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies4unrelated_withforeign) +{ + TestSortByLoadDependenciesUnrelated(4, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies4related_withforeign) +{ + TestSortByLoadDependenciesRelated(4, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies5unrelated_withforeign) +{ + TestSortByLoadDependenciesUnrelated(5, true); +} + +BOOST_AUTO_TEST_CASE(sortbyloaddependencies5related_withforeign) +{ + TestSortByLoadDependenciesRelated(5, true); +} + BOOST_AUTO_TEST_SUITE_END()