From 6a464cf9f2cff34763527e4880b116d81f9b9011 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Tue, 9 Jul 2024 13:54:46 +0200 Subject: [PATCH 01/25] Parse config from new flexible YAML format. --- README.md | 4 +- examples/config/sample-first-datasource.toml | 13 --- examples/config/sample-first-datasource.yaml | 13 +++ examples/config/sample-second-datasource.toml | 14 ---- examples/config/sample-second-datasource.yaml | 13 +++ examples/config/sample-service.toml | 19 ----- examples/config/sample-service.yaml | 15 ++++ libs/http-service/src/cli.cpp | 82 +++++++++++++++++++ 8 files changed, 125 insertions(+), 48 deletions(-) delete mode 100644 examples/config/sample-first-datasource.toml create mode 100644 examples/config/sample-first-datasource.yaml delete mode 100644 examples/config/sample-second-datasource.toml create mode 100644 examples/config/sample-second-datasource.yaml delete mode 100644 examples/config/sample-service.toml create mode 100644 examples/config/sample-service.yaml diff --git a/README.md b/README.md index b0231891..b8b04bca 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,8 @@ The `mapget` executable can parse a config file with arguments supported by the Sample configuration files can be found under `examples/config`: -- [sample-first-datasource.toml](examples/config/sample-first-datasource.toml) and [sample-second-datasource.toml](examples/config/sample-second-datasource.toml) will configure mapget to run a simple datasource with sample data. Note: the two formats in config files for subcommand parameters can be used interchangeably. -- [sample-service.toml](examples/config/sample-service.toml) to execute the `mapget serve` command. The instance will fetch and serve data from sources started with `sample-*-datasource.toml` configs above. +- [sample-first-datasource.toml](examples/config/sample-first-datasource.yaml) and [sample-second-datasource.toml](examples/config/sample-second-datasource.yaml) will configure mapget to run a simple datasource with sample data. Note: the two formats in config files for subcommand parameters can be used interchangeably. +- [sample-service.toml](examples/config/sample-service.yaml) to execute the `mapget serve` command. The instance will fetch and serve data from sources started with `sample-*-datasource.toml` configs above. ### Cache diff --git a/examples/config/sample-first-datasource.toml b/examples/config/sample-first-datasource.toml deleted file mode 100644 index 9da7d419..00000000 --- a/examples/config/sample-first-datasource.toml +++ /dev/null @@ -1,13 +0,0 @@ -# mapget: A client/server application for map data retrieval. - -# From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. -log-level="trace" - -# Server to connect to in format . -fetch.server="127.0.0.1:61852" -# Map to retrieve. -fetch.map="Tropico" -# Layer of the map to retrieve. -fetch.layer="WayLayer" -# Tile of the map to retrieve. Can be specified multiple times. -fetch.tile="12345" diff --git a/examples/config/sample-first-datasource.yaml b/examples/config/sample-first-datasource.yaml new file mode 100644 index 00000000..12c2ca03 --- /dev/null +++ b/examples/config/sample-first-datasource.yaml @@ -0,0 +1,13 @@ +# mapget: A client/server application for map data retrieval. +mapget: + # From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. + log-level: trace + fetch: + # Server to connect to in format . + server: 127.0.0.1:61852 + # Map to retrieve. + map: Tropico + # Layer of the map to retrieve. + layer: WayLayer + # Tile of the map to retrieve. Can be specified multiple times. + tile: 12345 diff --git a/examples/config/sample-second-datasource.toml b/examples/config/sample-second-datasource.toml deleted file mode 100644 index 003e5b38..00000000 --- a/examples/config/sample-second-datasource.toml +++ /dev/null @@ -1,14 +0,0 @@ -# mapget: A client/server application for map data retrieval. - -# From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. -log-level="trace" - -[fetch] -# Server to connect to in format . -server="127.0.0.1:61852" -# Map to retrieve. -map="TestMap" -# Layer of the map to retrieve. -layer="WayLayer" -# Tile of the map to retrieve. Can be specified multiple times. -tile="12345" diff --git a/examples/config/sample-second-datasource.yaml b/examples/config/sample-second-datasource.yaml new file mode 100644 index 00000000..e411d9b1 --- /dev/null +++ b/examples/config/sample-second-datasource.yaml @@ -0,0 +1,13 @@ +# mapget: A client/server application for map data retrieval. +mapget: + # From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. + log-level: trace + fetch: + # Server to connect to in format . + server: 127.0.0.1:61852 + # Map to retrieve. + map: TestMap + # Layer of the map to retrieve. + layer: WayLayer + # Tile of the map to retrieve. Can be specified multiple times. + tile: 12345 diff --git a/examples/config/sample-service.toml b/examples/config/sample-service.toml deleted file mode 100644 index 85b1e3ec..00000000 --- a/examples/config/sample-service.toml +++ /dev/null @@ -1,19 +0,0 @@ -# mapget: A client/server application for map data retrieval. - -# From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. -log-level="trace" - -# Port to start the server on. Default is 0. -serve.port=61852 - -# Datasources for the server in format . Can be specified multiple times. -serve.datasource-host=["127.0.0.1:61853", "127.0.0.1:61854"] - -# Using a persistent cache. -serve.cache-type = "rocksdb" - -# Datasource executable paths, including arguments, for the server. Can be specified multiple times. -# serve.datasource-exe= - -# Serve a static web application, in the format [:]. -# serve.webapp= diff --git a/examples/config/sample-service.yaml b/examples/config/sample-service.yaml new file mode 100644 index 00000000..d85ea932 --- /dev/null +++ b/examples/config/sample-service.yaml @@ -0,0 +1,15 @@ +# mapget: A client/server application for map data retrieval. +mapget: + # From: trace, debug, info, warn, error, critical. Overrides MAPGET_LOG_LEVEL. + log-level: trace + serve: + # Port to start the server on. Default is 0. + port: 61852 + # Datasources for the server in format . Can be specified multiple times. + datasource-host: ["127.0.0.1:61853", "127.0.0.1:61854"] + # Using a persistent cache. + cache-type: rocksdb + # Datasource executable paths, including arguments, for the server. Can be specified multiple times. + # datasource-exe: ... + # Serve a static web application, in the format [:]. + # webapp: ... diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index 9ce5c63d..05f87983 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -9,10 +9,91 @@ #include #include #include +#include namespace mapget { +namespace +{ + +class ConfigYAML : public CLI::Config { +public: + std::string to_config(const CLI::App *app, bool default_also, bool, std::string) const override { + std::string config_path = app->get_config_ptr() ? app->get_config_ptr()->as() : "config.yaml"; + std::ifstream ifs(config_path); + YAML::Node root = ifs ? YAML::Load(ifs) : YAML::Node(); + + // Create or clear the 'mapget' node + auto mapgetNode = root["mapget"] = YAML::Node(YAML::NodeType::Map); + + // Process current app configuration into 'mapget' node + _to_yaml(mapgetNode, app, default_also); + + // Output the YAML content as a formatted string + std::stringstream ss; + ss << root; + return ss.str(); + } + + void _to_yaml(YAML::Node root, const CLI::App *app, bool default_also) const { + for (const CLI::Option *opt : app->get_options({})) { + if (!opt->get_lnames().empty() && opt->get_configurable()) { + std::string name = opt->get_lnames()[0]; + + if (opt->get_type_size() != 0) { + if (opt->count() > 0) + root[name] = opt->count() == 1 ? opt->results().at(0) : opt->results(); + else if (default_also && !opt->get_default_str().empty()) + root[name] = opt->get_default_str(); + } else { + root[name] = opt->count() ? (opt->count() > 1 ? opt->count() : true) : (default_also ? false : YAML::Node()); + } + } + } + + for (const CLI::App *subcom : app->get_subcommands({})) + _to_yaml(root[subcom->get_name()], subcom, default_also); + } + + std::vector from_config(std::istream &input) const override { + YAML::Node root = YAML::Load(input); + YAML::Node mapgetNode = root["mapget"]; + return mapgetNode ? _from_yaml(mapgetNode) : std::vector(); + } + + std::vector _from_yaml(const YAML::Node &node, const std::string &name = "", const std::vector &prefix = {}) const { + std::vector results; + + if (node.IsMap()) { + for (const auto &item : node) { + auto copy_prefix = prefix; + if (!name.empty()) { + copy_prefix.push_back(name); + } + auto sub_results = _from_yaml(item.second, item.first.as(), copy_prefix); + results.insert(results.end(), sub_results.begin(), sub_results.end()); + } + } else if (!name.empty()) { + results.emplace_back(); + CLI::ConfigItem &res = results.back(); + res.name = name; + res.parents = prefix; + if (node.IsScalar()) { + res.inputs = {node.as()}; + } else if (node.IsSequence()) { + for (const auto &val : node) { + res.inputs.push_back(val.as()); + } + } + } + + return results; + } +}; + +} + struct ServeCommand { int port_ = 0; @@ -197,6 +278,7 @@ int runFromCommandLine(std::vector args) "--config", "", "Optional path to a file with configuration arguments for mapget."); + app.config_formatter(std::make_shared()); app.require_subcommand(1); From 4528da09fce5ae55fc64df1be3485dab04c4f023 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Tue, 9 Jul 2024 14:13:13 +0200 Subject: [PATCH 02/25] Small fixes to YAML processing. --- CMakeLists.txt | 2 +- deps.cmake | 2 +- libs/http-service/src/cli.cpp | 14 ++++++++++---- test/integration/CMakeLists.txt | 12 ++++++------ 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f5f26950..9cb328b4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,7 +4,7 @@ cmake_policy(SET CMP0117 NEW) project(mapget CXX) -set(MAPGET_VERSION 2024.3) +set(MAPGET_VERSION 2024.3.1) set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) diff --git a/deps.cmake b/deps.cmake index 1fb00e7f..81fccbe6 100644 --- a/deps.cmake +++ b/deps.cmake @@ -126,7 +126,7 @@ if (MAPGET_WITH_WHEEL) FetchContent_MakeAvailable(python-cmake-wheel) endif() -set(BUILD_TESTING NO CACHE BOOL "") +set(BUILD_TESTING OFF CACHE BOOL "" FORCE) FetchContent_Declare(tiny-process-library GIT_REPOSITORY "https://gitlab.com/eidheim/tiny-process-library" GIT_TAG v2.0.4 diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index 05f87983..e827b2fd 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -42,12 +42,18 @@ class ConfigYAML : public CLI::Config { std::string name = opt->get_lnames()[0]; if (opt->get_type_size() != 0) { - if (opt->count() > 0) - root[name] = opt->count() == 1 ? opt->results().at(0) : opt->results(); + if (opt->count() == 1) + root[name] = opt->results().at(0); + else if (opt->count() > 0) + root[name] = opt->results(); else if (default_also && !opt->get_default_str().empty()) root[name] = opt->get_default_str(); - } else { - root[name] = opt->count() ? (opt->count() > 1 ? opt->count() : true) : (default_also ? false : YAML::Node()); + } + else if (opt->count()) { + root[name] = opt->count() > 1 ? YAML::Node(opt->count()) : YAML::Node(true); + } + else { + root[name] = default_also ? YAML::Node(false) : YAML::Node(); } } } diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index 9487db5f..f19ab35c 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -72,13 +72,13 @@ add_wheel_test(test-config-cpp -b "./cpp-sample-http-datasource ${DATASOURCE_CPP_PORT}" # Run service - -b "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-service.toml serve" + -b "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-service.yaml serve" # Request from py datasource - -f "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-second-datasource.toml fetch" + -f "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-second-datasource.yaml fetch" # Request from cpp datasource - -f "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-first-datasource.toml fetch" + -f "./mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-first-datasource.yaml fetch" ) add_wheel_test(test-config-py @@ -92,13 +92,13 @@ add_wheel_test(test-config-py -b "./cpp-sample-http-datasource ${DATASOURCE_CPP_PORT}" # Run service - -b "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-service.toml serve" + -b "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-service.yaml serve" # Request from py datasource - -f "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-second-datasource.toml fetch" + -f "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-second-datasource.yaml fetch" # Request from cpp datasource - -f "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-first-datasource.toml fetch" + -f "python -m mapget --config ${CMAKE_CURRENT_LIST_DIR}/../../examples/config/sample-first-datasource.yaml fetch" ) # TODO should there be a Python test for the 400 response? From aabe7c56899122cf68d2ddd40b40cdaf5b8dd6f9 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Tue, 9 Jul 2024 18:12:17 +0200 Subject: [PATCH 03/25] Ensure that Node 16 is used when building for manylinux. --- .github/workflows/cmake.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cmake.yaml b/.github/workflows/cmake.yaml index 5d4416ae..1a20ac25 100644 --- a/.github/workflows/cmake.yaml +++ b/.github/workflows/cmake.yaml @@ -12,8 +12,9 @@ jobs: matrix: python-version: ["3.8", "3.9", "3.10", "3.11"] runs-on: ubuntu-latest - container: ghcr.io/klebert-engineering/manylinux-cpp17-py${{ matrix.python-version }}:2023.1 + container: ghcr.io/klebert-engineering/manylinux-cpp17-py${{ matrix.python-version }}-x86_64:2024.1 env: + ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true SCCACHE_GHA_ENABLED: "true" steps: - uses: actions/checkout@v2 From f8a5cbaef155fa823276d556ecdd4d00e2d09c2e Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Wed, 10 Jul 2024 11:53:51 +0200 Subject: [PATCH 04/25] Fix ctest flag --no-tests=error. --- .github/workflows/cmake.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cmake.yaml b/.github/workflows/cmake.yaml index 1a20ac25..a91f3587 100644 --- a/.github/workflows/cmake.yaml +++ b/.github/workflows/cmake.yaml @@ -49,7 +49,7 @@ jobs: timeout-minutes: 30 run: | . ./venv/bin/activate - ctest --preset conan-release -C Release --verbose --no-test=fail + ctest --preset conan-release -C Release --verbose --no-tests=error - name: Deploy uses: actions/upload-artifact@v2 with: @@ -115,4 +115,4 @@ jobs: - name: Test timeout-minutes: 30 run: | - ctest --preset conan-release -C Release --verbose --no-test=fail + ctest --preset conan-release -C Release --verbose --no-tests=error From bf7a893e276d731f67a6603d1e040f2206e2289f Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Wed, 10 Jul 2024 12:29:57 +0200 Subject: [PATCH 05/25] Activate tests on macOS. --- .github/workflows/cmake.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/cmake.yaml b/.github/workflows/cmake.yaml index a91f3587..2a7fe463 100644 --- a/.github/workflows/cmake.yaml +++ b/.github/workflows/cmake.yaml @@ -90,6 +90,7 @@ jobs: -DPython3_FIND_FRAMEWORK=LAST \ -DCMAKE_C_COMPILER_LAUNCHER=sccache \ -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ + -DMAPGET_ENABLE_TESTING=ON \ -GNinja cmake --build --preset conan-release cd build/Release From 55867dee14ed6f1056861319a017f35091052349 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Wed, 10 Jul 2024 17:46:19 +0200 Subject: [PATCH 06/25] Add logic for watching the datasources in the config file. --- libs/service/CMakeLists.txt | 7 +- libs/service/include/mapget/service/config.h | 134 ++++++++++++++++ libs/service/include/mapget/service/service.h | 7 +- libs/service/src/config.cpp | 145 ++++++++++++++++++ libs/service/src/service.cpp | 81 +++++++++- 5 files changed, 369 insertions(+), 5 deletions(-) create mode 100644 libs/service/include/mapget/service/config.h create mode 100644 libs/service/src/config.cpp diff --git a/libs/service/CMakeLists.txt b/libs/service/CMakeLists.txt index 3e015170..5b270878 100644 --- a/libs/service/CMakeLists.txt +++ b/libs/service/CMakeLists.txt @@ -7,13 +7,15 @@ add_library(mapget-service STATIC include/mapget/service/memcache.h include/mapget/service/rocksdbcache.h include/mapget/service/locate.h + include/mapget/service/config.h src/service.cpp src/cache.cpp src/datasource.cpp src/memcache.cpp src/rocksdbcache.cpp - src/locate.cpp) + src/locate.cpp + src/config.cpp) target_include_directories(mapget-service PUBLIC @@ -25,7 +27,8 @@ target_link_libraries(mapget-service PUBLIC mapget-model mapget-log - RocksDB::rocksdb) + RocksDB::rocksdb + yaml-cpp::yaml-cpp) if (MSVC) target_compile_definitions(mapget-service diff --git a/libs/service/include/mapget/service/config.h b/libs/service/include/mapget/service/config.h new file mode 100644 index 00000000..fba23208 --- /dev/null +++ b/libs/service/include/mapget/service/config.h @@ -0,0 +1,134 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "datasource.h" +#include "yaml-cpp/yaml.h" + +namespace mapget +{ + +/** + * Singleton class that watches a particular YAML config file path. + * The config YAML must have a top-level `sources:` key, which hosts + * a list of datasource descriptors. Each descriptor must have a `type:` + * key, to describe the datasource constructor that is supposed to be called. + * The whole descriptor will be passed into the lambda that is registered + * as the constructor for the given type name when calling instantiate(). + * Services will call subscribe() to be notified about the currently + * active set of services from the config. + */ +class DataSourceConfig +{ +public: + /** + * Gets the singleton instance of the DataSourceConfig class. + * @return Reference to the singleton instance. + */ + static DataSourceConfig& instance(); + + /** + * Class representing a subscription to the configuration changes. + */ + class Subscription + { + public: + /** + * Destructor that ensures unsubscription. + */ + ~Subscription(); + + private: + uint32_t id_ = 0; + friend class DataSourceConfig; + }; + + /** + * Subscribes to configuration changes. + * The callback will be triggered once immediately, then whenever + * the config file path or content changes. + * @param callback Function to call with the current set of service config nodes. + * @return Unique pointer to a Subscription object. + */ + std::unique_ptr subscribe( + std::function const& serviceConfigNodes)> const& callback); + + /** + * Sets the path to the YAML configuration file to watch. + * @param path The file path to the YAML configuration file. + */ + void setConfigFilePath(std::string const& path); + + /** + * Instantiates a data source based on the provided descriptor. + * @param descriptor The YAML node containing the data source descriptor. + * @return Shared pointer to the instantiated data source, or nullptr if instantiation failed. + */ + DataSource::Ptr instantiate(YAML::Node const& descriptor); + + /** + * Registers a constructor for a given data source type. + * @param typeName The name of the data source type. + * @param constructor The constructor function to call for this data source type. + */ + void registerConstructor( + std::string const& typeName, + std::function constructor); + +private: + // Private constructor to enforce the singleton pattern. + DataSourceConfig(); + + // Destructor to clean up resources. + ~DataSourceConfig(); + + /** + * Unsubscribes a subscription based on its ID. + * @param id The subscription ID to remove. + */ + void unsubscribe(uint32_t id); + + /** + * Loads the configuration from the file. + */ + void loadConfig(); + + /** + * Starts watching the configuration file for changes. + * @param path The file path to the YAML configuration file. + */ + void restartFileWatchThread(); + + // Path to the configuration file. + std::string configFilePath_; + + // Map of subscription IDs to their respective callback functions. + std::unordered_map const&)>> subscriptions_; + + // Map of data source type names to their respective constructor functions. + std::unordered_map> constructors_; + + // Current configuration nodes. + std::vector currentConfig_; + + // Next available subscription ID. + uint32_t nextSubscriptionId_ = 0; + + // Atomic flag to control the file watching thread. + std::atomic watching_ = false; + + // Thread which is watching the config file changed-timestamp. + std::optional watchThread_; + + // Mutex to ensure that currentConfig_ and subscriptions_ are safely accessed. + std::recursive_mutex memberAccessMutex_; +}; + +} // namespace mapget diff --git a/libs/service/include/mapget/service/service.h b/libs/service/include/mapget/service/service.h index f7574579..33394416 100644 --- a/libs/service/include/mapget/service/service.h +++ b/libs/service/include/mapget/service/service.h @@ -102,8 +102,13 @@ class Service * Construct a service with a shared Cache instance. Note: The Cache must not * be null. For a simple default cache implementation, you can use the * MemCache. + * @param cache Cache instance to use. + * @param useDataSourceConfig Instruct this service instance to instantiate its datasource + * backends based on a subscription to the YAML datasource config file. */ - explicit Service(Cache::Ptr cache = std::make_shared()); + explicit Service( + Cache::Ptr cache = std::make_shared(), + bool useDataSourceConfig = false); /** Destructor. Stops all workers of the present data sources. */ ~Service(); diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp new file mode 100644 index 00000000..14e5aeed --- /dev/null +++ b/libs/service/src/config.cpp @@ -0,0 +1,145 @@ + +#include +#include +#include +#include + +#include "config.h" +#include "mapget/log.h" + +namespace mapget +{ + +DataSourceConfig& DataSourceConfig::instance() +{ + static DataSourceConfig instance; + return instance; +} + +DataSourceConfig::Subscription::~Subscription() +{ + DataSourceConfig::instance().unsubscribe(id_); +} + +std::unique_ptr +DataSourceConfig::subscribe(std::function const&)> const& callback) +{ + if (!callback) { + log().warn("Refusing to register config subscription with NULL callback."); + return nullptr; + } + + std::lock_guard memberAccessLock(memberAccessMutex_); + auto sub = std::make_unique(); + sub->id_ = nextSubscriptionId_++; + subscriptions_[sub->id_] = callback; + // Optionally, trigger the callback with the current configuration immediately + if (!currentConfig_.empty()) { + callback(currentConfig_); + } + return sub; +} + +void DataSourceConfig::unsubscribe(uint32_t id) +{ + std::lock_guard memberAccessLock(memberAccessMutex_); + subscriptions_.erase(id); +} + +void DataSourceConfig::setConfigFilePath(std::string const& path) { + configFilePath_ = path; + loadConfig(); // Initial load + restartFileWatchThread(); +} + +void DataSourceConfig::loadConfig() +{ + try { + YAML::Node config = YAML::LoadFile(configFilePath_); + if (auto sourcesNode = config["sources"]) { + std::lock_guard memberAccessLock(memberAccessMutex_); + currentConfig_.clear(); + for (auto const& node : sourcesNode) + currentConfig_.push_back(node); + for (const auto& subscriber : subscriptions_) { + subscriber.second(currentConfig_); + } + } + else { + log().warn("The config file {} does not have a sources node."); + } + } + catch (const YAML::Exception& e) { + log().error("Failed to load YAML config {}: {}", configFilePath_, e.what()); + } +} + +DataSource::Ptr DataSourceConfig::instantiate(YAML::Node const& descriptor) +{ + if (auto typeNode = descriptor["type"]) { + std::lock_guard memberAccessLock(memberAccessMutex_); + auto type = typeNode.as(); + auto it = constructors_.find(type); + if (it != constructors_.end()) { + if (auto result = it->second(descriptor)) { + return result; + } + log().error("Datasource constructor for type {} returned NULL.", type); + return nullptr; + } + log().error("No constructor registered for datasource type: ", type); + return nullptr; + } + log().error("A YAML datasource descriptor is missing the `type` key!"); + return nullptr; +} + +void DataSourceConfig::registerConstructor( + std::string const& typeName, + std::function constructor) +{ + if (!constructor) { + log().warn("Refusing to register NULL constructor for datasource type {}", typeName); + return; + } + std::lock_guard memberAccessLock(memberAccessMutex_); + constructors_[typeName] = std::move(constructor); +} + +void DataSourceConfig::restartFileWatchThread() +{ + namespace fs = std::filesystem; + + if (watchThread_) { + // If there's already a thread running, wait for it to finish before starting a new one. + watching_ = false; + watchThread_->join(); + watching_ = true; + } + + watchThread_ = std::thread( + [this, path= configFilePath_]() + { + auto lastModTime = fs::last_write_time(path); + while (watching_) { + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + auto currentModTime = fs::last_write_time(path); + if (currentModTime != lastModTime) { + loadConfig(); + lastModTime = currentModTime; + } + } + }); +} + +DataSourceConfig::DataSourceConfig() = default; + +DataSourceConfig::~DataSourceConfig() { + // Signal the watching thread to stop. + watching_ = false; + if (watchThread_ && watchThread_->joinable()) { + watchThread_->join(); // Wait for the thread to finish. + } +} + +} // namespace mapget diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index a5e37699..c5c394db 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -1,6 +1,7 @@ #include "service.h" #include "locate.h" #include "mapget/log.h" +#include "config.h" #include #include @@ -257,10 +258,77 @@ struct Service::Impl : public Service::Controller std::map> dataSourceWorkers_; std::list addOnDataSources_; - explicit Impl(Cache::Ptr cache) : Controller(std::move(cache)) {} + std::unique_ptr configSubscription_; + std::map dataSourceConfigs_; + + explicit Impl(Cache::Ptr cache, bool useDataSourceConfig) : Controller(std::move(cache)) + { + if (!useDataSourceConfig) + return; + configSubscription_ = DataSourceConfig::instance().subscribe( + [this](auto&& dataSourceConfigNodes) + { + log().info("Config changed. Scanning for datasource changes..."); + + // Deserialize datasource configurations and update service accordingly + std::map newConfigs; + for (const auto& node : dataSourceConfigNodes) { + YAML::Emitter out; + out << YAML::DoubleQuoted << YAML::Flow << node; + std::string serializedConfig = out.c_str(); + + if (!out.good()) { + log().error("YAML serialization failed: {}", out.GetLastError()); + continue; + } + + newConfigs[serializedConfig] = node; + } + + // Remove datasources not present in the new configuration + auto it = dataSourceConfigs_.begin(); + while (it != dataSourceConfigs_.end()) { + if (newConfigs.find(it->first) == newConfigs.end()) { + log().info("Removing datasource with config: {}", it->first); + removeDataSource(it->second); + it = dataSourceConfigs_.erase(it); + } + else { + ++it; + } + } + + // Add or update datasources present in the new configuration + for (const auto& [configKey, configNode] : newConfigs) { + if (dataSourceConfigs_.find(configKey) == dataSourceConfigs_.end()) { + log().info("Adding new datasource with config: {}", configKey); + auto dataSource = DataSourceConfig::instance().instantiate(configNode); + if (dataSource) { + addDataSource(dataSource); + dataSourceConfigs_[configKey] = dataSource; + } + else { + log().error( + "Failed to instantiate datasource with config: {}", + configKey); + } + } + else { + // Potentially update existing datasource if needed + log().info( + "Datasource already exists, no update required for config: {}", + configKey); + // Optional: Handle updates to existing datasources + } + } + }); + } ~Impl() { + // Ensure that no new datasources are added while we are cleaning up. + configSubscription_.reset(); + for (auto& dataSourceAndWorkers : dataSourceWorkers_) { for (auto& worker : dataSourceAndWorkers.second) { worker->shouldTerminate_ = true; @@ -453,17 +521,26 @@ struct Service::Impl : public Service::Controller } }; -Service::Service(Cache::Ptr cache) : impl_(std::make_unique(std::move(cache))) {} +Service::Service(Cache::Ptr cache, bool useDataSourceConfig) + : impl_(std::make_unique(std::move(cache), useDataSourceConfig)) +{ +} Service::~Service() = default; void Service::add(DataSource::Ptr const& dataSource) { + if (impl_->configSubscription_) { + raise("Service::add() cannot be used: Datasources are managed via config file."); + } impl_->addDataSource(dataSource); } void Service::remove(const DataSource::Ptr& dataSource) { + if (impl_->configSubscription_) { + raise("Service::remove() cannot be used: Datasources are managed via config file."); + } impl_->removeDataSource(dataSource); } From e47aee25a9c09441242d1ad4e7bef234036c51be Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 11 Jul 2024 09:31:37 +0200 Subject: [PATCH 07/25] Small bugfixes. --- .github/workflows/coverage.yml | 2 +- libs/service/src/service.cpp | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index e522a189..a1522240 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -42,7 +42,7 @@ jobs: cmake --build --preset conan-debug - name: Run Test run: | - ctest --preset conan-debug -C Debug --verbose --no-test=fail + ctest --preset conan-debug -C Debug --verbose --no-tests=error - name: Run Gcovr run: | mkdir coverage diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index c5c394db..99105471 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -314,11 +314,9 @@ struct Service::Impl : public Service::Controller } } else { - // Potentially update existing datasource if needed log().info( "Datasource already exists, no update required for config: {}", configKey); - // Optional: Handle updates to existing datasources } } }); @@ -530,17 +528,11 @@ Service::~Service() = default; void Service::add(DataSource::Ptr const& dataSource) { - if (impl_->configSubscription_) { - raise("Service::add() cannot be used: Datasources are managed via config file."); - } impl_->addDataSource(dataSource); } void Service::remove(const DataSource::Ptr& dataSource) { - if (impl_->configSubscription_) { - raise("Service::remove() cannot be used: Datasources are managed via config file."); - } impl_->removeDataSource(dataSource); } From 028ace976d493e87d9ea4fb12745f53f2860a7bf Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 11 Jul 2024 15:50:29 +0200 Subject: [PATCH 08/25] Added config service test and essential bugfixes. --- libs/service/include/mapget/service/config.h | 21 ++-- libs/service/include/mapget/service/service.h | 4 + libs/service/src/config.cpp | 104 ++++++++++++----- libs/service/src/service.cpp | 11 +- test/unit/CMakeLists.txt | 3 +- test/unit/test-config.cpp | 106 ++++++++++++++++++ 6 files changed, 210 insertions(+), 39 deletions(-) create mode 100644 test/unit/test-config.cpp diff --git a/libs/service/include/mapget/service/config.h b/libs/service/include/mapget/service/config.h index fba23208..3affbfbc 100644 --- a/libs/service/include/mapget/service/config.h +++ b/libs/service/include/mapget/service/config.h @@ -7,7 +7,7 @@ #include #include #include -#include +#include #include #include "datasource.h" #include "yaml-cpp/yaml.h" @@ -25,14 +25,14 @@ namespace mapget * Services will call subscribe() to be notified about the currently * active set of services from the config. */ -class DataSourceConfig +class DataSourceConfigService { public: /** * Gets the singleton instance of the DataSourceConfig class. * @return Reference to the singleton instance. */ - static DataSourceConfig& instance(); + static DataSourceConfigService& get(); /** * Class representing a subscription to the configuration changes. @@ -47,7 +47,7 @@ class DataSourceConfig private: uint32_t id_ = 0; - friend class DataSourceConfig; + friend class DataSourceConfigService; }; /** @@ -82,12 +82,17 @@ class DataSourceConfig std::string const& typeName, std::function constructor); + /** + * Call this to stop the config file watching thread. + */ + void end(); + private: // Private constructor to enforce the singleton pattern. - DataSourceConfig(); + DataSourceConfigService(); // Destructor to clean up resources. - ~DataSourceConfig(); + ~DataSourceConfigService(); /** * Unsubscribes a subscription based on its ID. @@ -110,10 +115,10 @@ class DataSourceConfig std::string configFilePath_; // Map of subscription IDs to their respective callback functions. - std::unordered_map const&)>> subscriptions_; + std::map const&)>> subscriptions_; // Map of data source type names to their respective constructor functions. - std::unordered_map> constructors_; + std::map> constructors_; // Current configuration nodes. std::vector currentConfig_; diff --git a/libs/service/include/mapget/service/service.h b/libs/service/include/mapget/service/service.h index 33394416..ccb873a1 100644 --- a/libs/service/include/mapget/service/service.h +++ b/libs/service/include/mapget/service/service.h @@ -118,6 +118,8 @@ class Service * and incoming/present requests for the data source will start to be * processed. Note, that the map layer versions for all layers of the * given source must be compatible with present one's, if existing. + * + * Thread safety: This method should not be called in parallel with itself or remove(). */ void add(DataSource::Ptr const& dataSource); @@ -125,6 +127,8 @@ class Service * Remove a data source from the service. Requests for data which * can only be satisfied by the given source will not be processed anymore. * TODO: Any such ongoing requests should be forcefully marked as done. + * + * Thread safety: This method should not be called in parallel with itself or add(). */ void remove(DataSource::Ptr const& dataSource); diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index 14e5aeed..f7d4e596 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "config.h" #include "mapget/log.h" @@ -10,19 +11,19 @@ namespace mapget { -DataSourceConfig& DataSourceConfig::instance() +DataSourceConfigService& DataSourceConfigService::get() { - static DataSourceConfig instance; + static DataSourceConfigService instance; return instance; } -DataSourceConfig::Subscription::~Subscription() +DataSourceConfigService::Subscription::~Subscription() { - DataSourceConfig::instance().unsubscribe(id_); + DataSourceConfigService::get().unsubscribe(id_); } -std::unique_ptr -DataSourceConfig::subscribe(std::function const&)> const& callback) +std::unique_ptr DataSourceConfigService::subscribe( + std::function const&)> const& callback) { if (!callback) { log().warn("Refusing to register config subscription with NULL callback."); @@ -40,19 +41,19 @@ DataSourceConfig::subscribe(std::function const&)> return sub; } -void DataSourceConfig::unsubscribe(uint32_t id) +void DataSourceConfigService::unsubscribe(uint32_t id) { std::lock_guard memberAccessLock(memberAccessMutex_); subscriptions_.erase(id); } -void DataSourceConfig::setConfigFilePath(std::string const& path) { +void DataSourceConfigService::setConfigFilePath(std::string const& path) +{ configFilePath_ = path; - loadConfig(); // Initial load restartFileWatchThread(); } -void DataSourceConfig::loadConfig() +void DataSourceConfigService::loadConfig() { try { YAML::Node config = YAML::LoadFile(configFilePath_); @@ -61,8 +62,9 @@ void DataSourceConfig::loadConfig() currentConfig_.clear(); for (auto const& node : sourcesNode) currentConfig_.push_back(node); - for (const auto& subscriber : subscriptions_) { - subscriber.second(currentConfig_); + for (const auto& [subId, subCb] : subscriptions_) { + log().debug("Calling subscriber {}", subId); + subCb(currentConfig_); } } else { @@ -74,7 +76,7 @@ void DataSourceConfig::loadConfig() } } -DataSource::Ptr DataSourceConfig::instantiate(YAML::Node const& descriptor) +DataSource::Ptr DataSourceConfigService::instantiate(YAML::Node const& descriptor) { if (auto typeNode = descriptor["type"]) { std::lock_guard memberAccessLock(memberAccessMutex_); @@ -87,14 +89,14 @@ DataSource::Ptr DataSourceConfig::instantiate(YAML::Node const& descriptor) log().error("Datasource constructor for type {} returned NULL.", type); return nullptr; } - log().error("No constructor registered for datasource type: ", type); + log().error("No constructor registered for datasource type: {}", type); return nullptr; } log().error("A YAML datasource descriptor is missing the `type` key!"); return nullptr; } -void DataSourceConfig::registerConstructor( +void DataSourceConfigService::registerConstructor( std::string const& typeName, std::function constructor) { @@ -106,7 +108,7 @@ void DataSourceConfig::registerConstructor( constructors_[typeName] = std::move(constructor); } -void DataSourceConfig::restartFileWatchThread() +void DataSourceConfigService::restartFileWatchThread() { namespace fs = std::filesystem; @@ -114,27 +116,79 @@ void DataSourceConfig::restartFileWatchThread() // If there's already a thread running, wait for it to finish before starting a new one. watching_ = false; watchThread_->join(); - watching_ = true; } + watching_ = true; watchThread_ = std::thread( - [this, path= configFilePath_]() + [this, path = configFilePath_]() { - auto lastModTime = fs::last_write_time(path); + auto toStr = [](auto&& time){ + std::stringstream ss; + ss << time; + return ss.str(); + }; + + log().debug("Starting watch thread for {}.", path); + + fs::file_time_type lastModTime; + bool fileExisted = fs::exists(path); + + if (fileExisted) { + lastModTime = fs::last_write_time(path); + log().debug("The config file exists (t={}).", toStr(lastModTime)); + loadConfig(); + } + else { + log().debug("The config file does not exist yet."); + } + while (watching_) { std::this_thread::sleep_for(std::chrono::milliseconds(500)); - auto currentModTime = fs::last_write_time(path); - if (currentModTime != lastModTime) { - loadConfig(); - lastModTime = currentModTime; + if (fs::exists(path)) { + if (!fileExisted) { + // The file has appeared since the last check. + lastModTime = fs::last_write_time(path); + log().debug("The config file exists now (t={}).", toStr(lastModTime)); + fileExisted = true; + loadConfig(); + } + else { + auto currentModTime = fs::last_write_time(path); + + if (currentModTime != lastModTime) { + // The file exists and has been modified since the last check. + log().debug( + "The config file changed ({} vs {}).", + toStr(currentModTime), + toStr(lastModTime)); + lastModTime = currentModTime; + loadConfig(); + } + else { + log().debug( + "The config file is unchanged ({} vs {}).", + toStr(currentModTime), + toStr(lastModTime)); + } + } + } + else if (fileExisted) { + log().debug("The config file disappeared."); + fileExisted = false; } } }); } -DataSourceConfig::DataSourceConfig() = default; +DataSourceConfigService::DataSourceConfigService() = default; + +DataSourceConfigService::~DataSourceConfigService() +{ + end(); +} -DataSourceConfig::~DataSourceConfig() { +void DataSourceConfigService::end() +{ // Signal the watching thread to stop. watching_ = false; if (watchThread_ && watchThread_->joinable()) { diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index 99105471..cf812f09 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -209,7 +209,9 @@ struct Service::Worker lock, [&, this]() { + log().trace("Worker checking conditions."); if (shouldTerminate_) { + log().trace("Terminating."); // Set by the controller at shutdown or if a data source // is removed. All worker instances are expected to terminate. return true; @@ -258,14 +260,14 @@ struct Service::Impl : public Service::Controller std::map> dataSourceWorkers_; std::list addOnDataSources_; - std::unique_ptr configSubscription_; + std::unique_ptr configSubscription_; std::map dataSourceConfigs_; explicit Impl(Cache::Ptr cache, bool useDataSourceConfig) : Controller(std::move(cache)) { if (!useDataSourceConfig) return; - configSubscription_ = DataSourceConfig::instance().subscribe( + configSubscription_ = DataSourceConfigService::get().subscribe( [this](auto&& dataSourceConfigNodes) { log().info("Config changed. Scanning for datasource changes..."); @@ -301,8 +303,8 @@ struct Service::Impl : public Service::Controller // Add or update datasources present in the new configuration for (const auto& [configKey, configNode] : newConfigs) { if (dataSourceConfigs_.find(configKey) == dataSourceConfigs_.end()) { - log().info("Adding new datasource with config: {}", configKey); - auto dataSource = DataSourceConfig::instance().instantiate(configNode); + log().info("Adding new datasource with config: `{}`", configKey); + auto dataSource = DataSourceConfigService::get().instantiate(configNode); if (dataSource) { addDataSource(dataSource); dataSourceConfigs_[configKey] = dataSource; @@ -381,7 +383,6 @@ struct Service::Impl : public Service::Controller void removeDataSource(DataSource::Ptr const& dataSource) { - std::unique_lock lock(jobsMutex_); dataSourceInfo_.erase(dataSource); addOnDataSources_.remove(dataSource); diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 2f629a64..36249495 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -18,7 +18,8 @@ add_executable(test.mapget test-simfil-geometry.cpp test-info.cpp test-http-datasource.cpp - test-cache.cpp) + test-cache.cpp + test-config.cpp) add_executable(test.mapget.filelog test-file-logging.cpp) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp new file mode 100644 index 00000000..4088178e --- /dev/null +++ b/test/unit/test-config.cpp @@ -0,0 +1,106 @@ +#include +#include +#include +#include +#include + +#include "mapget/service/service.h" +#include "mapget/service/datasource.h" +#include "mapget/service/config.h" +#include "mapget/log.h" +#include "mapget/service/memcache.h" + +namespace fs = std::filesystem; +using namespace mapget; + +struct TestDataSource : public DataSource +{ + DataSourceInfo info() override { + return DataSourceInfo::fromJson(R"( + { + "mapId": "Catan", + "layers": {} + } + )"_json); + }; + + void fill(TileFeatureLayer::Ptr const& featureTile) override {}; +}; + +std::string generateTimestampedDirectoryName(const std::string& baseName) { + auto now = std::chrono::system_clock::now(); + auto duration = now.time_since_epoch(); + auto millis = std::chrono::duration_cast(duration).count(); + + return baseName + "_" + std::to_string(millis); +} + +TEST_CASE("Load Config From File", "[DataSourceConfig]") +{ + setLogLevel("trace", log()); + + auto tempDir = fs::temp_directory_path() / generateTimestampedDirectoryName("mapget_test"); + fs::create_directory(tempDir); + auto tempConfigPath = tempDir / "temp_config.yaml"; + + DataSourceConfigService::get().registerConstructor( + "TestDataSource", + [](const YAML::Node& config) -> DataSource::Ptr { + return std::make_shared(); + }); + + auto cache = std::make_shared(); + Service service(cache, true); + REQUIRE(service.info().empty()); + + std::binary_semaphore semaphore(0); + + auto subscription = DataSourceConfigService::get().subscribe( + [&](auto&&) { + log().debug("Release the semaphore!"); + semaphore.release(); + }); + + DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); + + // Initial empty configuration + { + std::ofstream out(tempConfigPath, std::ios_base::trunc); + out << "sources: []" << std::endl; + } + REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); + REQUIRE(service.info().empty()); + log().debug("Gate passed!"); + + // Adding a datasource + std::this_thread::sleep_for(std::chrono::seconds(1)); + { + std::ofstream out(tempConfigPath, std::ios_base::trunc); + out << R"( + sources: + - type: TestDataSource + )" << std::endl; + } + REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); + log().debug("Gate passed!"); + + auto dataSourceInfos = service.info(); + REQUIRE(dataSourceInfos.size() == 1); + REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); + + // Removing the datasource + std::this_thread::sleep_for(std::chrono::seconds(1)); + { + std::ofstream out(tempConfigPath, std::ios_base::trunc); + out << "sources: []"; + } + REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); + REQUIRE(service.info().empty()); + log().debug("Gate passed!"); + + // Cleanup + fs::remove_all(tempDir); + // Wait for "The config file disappeared" message :) + std::this_thread::sleep_for(std::chrono::seconds(1)); + DataSourceConfigService::get().end(); +} From 6af5489e4f733b26b4c27f5b40186b018d4b6951 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 14:41:37 +0200 Subject: [PATCH 09/25] Integrate CLI with DataSourceConfigService. --- .../http-datasource/datasource-client.h | 5 +++ .../http-datasource/src/datasource-client.cpp | 9 ++++ .../mapget/http-service/http-service.h | 2 +- libs/http-service/src/cli.cpp | 41 +++++++++++++++---- libs/http-service/src/http-service.cpp | 3 +- libs/service/include/mapget/service/config.h | 6 +-- libs/service/include/mapget/service/service.h | 2 +- libs/service/src/config.cpp | 7 ++-- libs/service/src/service.cpp | 4 +- test/unit/test-config.cpp | 7 ++-- 10 files changed, 63 insertions(+), 23 deletions(-) diff --git a/libs/http-datasource/include/mapget/http-datasource/datasource-client.h b/libs/http-datasource/include/mapget/http-datasource/datasource-client.h index 747742a2..433a52fd 100644 --- a/libs/http-datasource/include/mapget/http-datasource/datasource-client.h +++ b/libs/http-datasource/include/mapget/http-datasource/datasource-client.h @@ -19,6 +19,11 @@ namespace mapget class RemoteDataSource : public DataSource { public: + /** + * Construct from joint host:port string. + */ + static std::shared_ptr fromHostPort(std::string const& hostPort); + /** * Construct a DataSource with the host and port of * a running DataSourceServer. Throws if the connection diff --git a/libs/http-datasource/src/datasource-client.cpp b/libs/http-datasource/src/datasource-client.cpp index 13232b7d..52aefe38 100644 --- a/libs/http-datasource/src/datasource-client.cpp +++ b/libs/http-datasource/src/datasource-client.cpp @@ -122,6 +122,15 @@ std::vector RemoteDataSource::locate(const LocateRequest& req) return responseVector; } +std::shared_ptr RemoteDataSource::fromHostPort(const std::string& hostPort) +{ + auto delimiterPos = hostPort.find(':'); + std::string dsHost = hostPort.substr(0, delimiterPos); + int dsPort = std::stoi(hostPort.substr(delimiterPos + 1, hostPort.size())); + log().info("Connecting to datasource at {}:{}.", dsHost, dsPort); + return std::make_shared(dsHost, dsPort); +} + RemoteDataSourceProcess::RemoteDataSourceProcess(std::string const& commandLine) { auto stderrCallback = [this](const char* bytes, size_t n) diff --git a/libs/http-service/include/mapget/http-service/http-service.h b/libs/http-service/include/mapget/http-service/http-service.h index cd9cb8e7..73e06d24 100644 --- a/libs/http-service/include/mapget/http-service/http-service.h +++ b/libs/http-service/include/mapget/http-service/http-service.h @@ -14,7 +14,7 @@ namespace mapget { class HttpService : public HttpServer, public Service { public: - explicit HttpService(Cache::Ptr cache = std::make_shared()); + explicit HttpService(Cache::Ptr cache = std::make_shared(), bool watchConfig = false); ~HttpService() override; protected: diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index e827b2fd..cede509a 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -5,6 +5,7 @@ #include "mapget/http-datasource/datasource-client.h" #include "mapget/service/rocksdbcache.h" +#include "mapget/service/config.h" #include #include @@ -68,7 +69,7 @@ class ConfigYAML : public CLI::Config { return mapgetNode ? _from_yaml(mapgetNode) : std::vector(); } - std::vector _from_yaml(const YAML::Node &node, const std::string &name = "", const std::vector &prefix = {}) const { + [[nodiscard]] std::vector _from_yaml(const YAML::Node &node, const std::string &name = "", const std::vector &prefix = {}) const { std::vector results; if (node.IsMap()) { @@ -98,6 +99,26 @@ class ConfigYAML : public CLI::Config { } }; +void registerDefaultDatasourceTypes() { + auto& config = DataSourceConfigService::get(); + config.registerDataSourceType( + "DataSourceHost", + [](YAML::Node const& config) -> DataSource::Ptr { + if (auto url = config["url"]) + return RemoteDataSource::fromHostPort(url.as()); + else + throw std::runtime_error("Missing `url` field."); + }); + config.registerDataSourceType( + "DataSourceProcess", + [](YAML::Node const& config) -> DataSource::Ptr { + if (auto cmd = config["cmd"]) + return std::make_shared(cmd.as()); + else + throw std::runtime_error("Missing `cmd` field."); + }); +}; + } struct ServeCommand @@ -110,8 +131,9 @@ struct ServeCommand int64_t cacheMaxTiles_ = 1024; bool clearCache_ = false; std::string webapp_; + CLI::App& app_; - explicit ServeCommand(CLI::App& app) + explicit ServeCommand(CLI::App& app) : app_(app) { auto serveCmd = app.add_subcommand("serve", "Starts the server."); serveCmd->add_option( @@ -163,16 +185,19 @@ struct ServeCommand raise(fmt::format("Cache type {} not supported!", cacheType_)); } - HttpService srv(cache); + bool watchConfig = false; + if (auto config = app_.get_config_ptr()) { + watchConfig = true; + registerDefaultDatasourceTypes(); + DataSourceConfigService::get().setConfigFilePath(config->as()); + }; + + HttpService srv(cache, watchConfig); if (!datasourceHosts_.empty()) { for (auto& ds : datasourceHosts_) { - auto delimiterPos = ds.find(':'); - std::string dsHost = ds.substr(0, delimiterPos); - int dsPort = std::stoi(ds.substr(delimiterPos + 1, ds.size())); - log().info("Connecting to datasource at {}:{}.", dsHost, dsPort); try { - srv.add(std::make_shared(dsHost, dsPort)); + srv.add(RemoteDataSource::fromHostPort(ds)); } catch (std::exception const& e) { log().error(" ...failed: {}", e.what()); diff --git a/libs/http-service/src/http-service.cpp b/libs/http-service/src/http-service.cpp index 45e1f0b0..75f46ac0 100644 --- a/libs/http-service/src/http-service.cpp +++ b/libs/http-service/src/http-service.cpp @@ -240,7 +240,8 @@ struct HttpService::Impl } }; -HttpService::HttpService(Cache::Ptr cache) : Service(std::move(cache)), impl_(std::make_unique(*this)) +HttpService::HttpService(Cache::Ptr cache, bool watchConfig) + : Service(std::move(cache), watchConfig), impl_(std::make_unique(*this)) {} HttpService::~HttpService() = default; diff --git a/libs/service/include/mapget/service/config.h b/libs/service/include/mapget/service/config.h index 3affbfbc..9818d4a0 100644 --- a/libs/service/include/mapget/service/config.h +++ b/libs/service/include/mapget/service/config.h @@ -21,7 +21,7 @@ namespace mapget * a list of datasource descriptors. Each descriptor must have a `type:` * key, to describe the datasource constructor that is supposed to be called. * The whole descriptor will be passed into the lambda that is registered - * as the constructor for the given type name when calling instantiate(). + * as the constructor for the given type name when calling makeDataSource(). * Services will call subscribe() to be notified about the currently * active set of services from the config. */ @@ -71,14 +71,14 @@ class DataSourceConfigService * @param descriptor The YAML node containing the data source descriptor. * @return Shared pointer to the instantiated data source, or nullptr if instantiation failed. */ - DataSource::Ptr instantiate(YAML::Node const& descriptor); + DataSource::Ptr makeDataSource(YAML::Node const& descriptor); /** * Registers a constructor for a given data source type. * @param typeName The name of the data source type. * @param constructor The constructor function to call for this data source type. */ - void registerConstructor( + void registerDataSourceType( std::string const& typeName, std::function constructor); diff --git a/libs/service/include/mapget/service/service.h b/libs/service/include/mapget/service/service.h index ccb873a1..1517339f 100644 --- a/libs/service/include/mapget/service/service.h +++ b/libs/service/include/mapget/service/service.h @@ -103,7 +103,7 @@ class Service * be null. For a simple default cache implementation, you can use the * MemCache. * @param cache Cache instance to use. - * @param useDataSourceConfig Instruct this service instance to instantiate its datasource + * @param useDataSourceConfig Instruct this service instance to makeDataSource its datasource * backends based on a subscription to the YAML datasource config file. */ explicit Service( diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index f7d4e596..44561329 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -76,7 +76,7 @@ void DataSourceConfigService::loadConfig() } } -DataSource::Ptr DataSourceConfigService::instantiate(YAML::Node const& descriptor) +DataSource::Ptr DataSourceConfigService::makeDataSource(YAML::Node const& descriptor) { if (auto typeNode = descriptor["type"]) { std::lock_guard memberAccessLock(memberAccessMutex_); @@ -96,7 +96,7 @@ DataSource::Ptr DataSourceConfigService::instantiate(YAML::Node const& descripto return nullptr; } -void DataSourceConfigService::registerConstructor( +void DataSourceConfigService::registerDataSourceType( std::string const& typeName, std::function constructor) { @@ -106,6 +106,7 @@ void DataSourceConfigService::registerConstructor( } std::lock_guard memberAccessLock(memberAccessMutex_); constructors_[typeName] = std::move(constructor); + log().info("Registered data source type {}.", typeName); } void DataSourceConfigService::restartFileWatchThread() @@ -165,7 +166,7 @@ void DataSourceConfigService::restartFileWatchThread() loadConfig(); } else { - log().debug( + log().trace( "The config file is unchanged ({} vs {}).", toStr(currentModTime), toStr(lastModTime)); diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index cf812f09..63371be8 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -304,14 +304,14 @@ struct Service::Impl : public Service::Controller for (const auto& [configKey, configNode] : newConfigs) { if (dataSourceConfigs_.find(configKey) == dataSourceConfigs_.end()) { log().info("Adding new datasource with config: `{}`", configKey); - auto dataSource = DataSourceConfigService::get().instantiate(configNode); + auto dataSource = DataSourceConfigService::get().makeDataSource(configNode); if (dataSource) { addDataSource(dataSource); dataSourceConfigs_[configKey] = dataSource; } else { log().error( - "Failed to instantiate datasource with config: {}", + "Failed to makeDataSource datasource with config: {}", configKey); } } diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 4088178e..2fd3a96f 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -43,11 +43,10 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") fs::create_directory(tempDir); auto tempConfigPath = tempDir / "temp_config.yaml"; - DataSourceConfigService::get().registerConstructor( + DataSourceConfigService::get().registerDataSourceType( "TestDataSource", - [](const YAML::Node& config) -> DataSource::Ptr { - return std::make_shared(); - }); + [](const YAML::Node& config) -> DataSource::Ptr + { return std::make_shared(); }); auto cache = std::make_shared(); Service service(cache, true); From 60474c274688641523d57261f57c4363371f5e1c Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 14:42:03 +0200 Subject: [PATCH 10/25] Deprecate serve -e and -d options. --- libs/http-service/src/cli.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index cede509a..6664ab53 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -141,15 +141,17 @@ struct ServeCommand port_, "Port to start the server on. Default is 0.") ->default_val("0"); - serveCmd->add_option( + CLI::deprecate_option(serveCmd->add_option( "-d,--datasource-host", datasourceHosts_, - "Data sources in format . Can be specified multiple times."); - serveCmd->add_option( + "This option is deprecated. Use a config file instead!. " + "Data sources in format . Can be specified multiple times.")); + CLI::deprecate_option(serveCmd->add_option( "-e,--datasource-exe", datasourceExecutables_, + "This option is deprecated. Use a config file instead!. " "Data source executable paths, including arguments. " - "Can be specified multiple times."); + "Can be specified multiple times.")); serveCmd->add_option( "-c,--cache-type", cacheType_, "From [memory|rocksdb], default memory, rocksdb (Technology Preview).") ->default_val("memory"); From 10ee3bbf69248fe86720785a09a8558603659f95 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 14:42:27 +0200 Subject: [PATCH 11/25] Catch any datasource constructor exception. --- libs/service/src/config.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index 44561329..175a6a51 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -83,13 +83,19 @@ DataSource::Ptr DataSourceConfigService::makeDataSource(YAML::Node const& descri auto type = typeNode.as(); auto it = constructors_.find(type); if (it != constructors_.end()) { - if (auto result = it->second(descriptor)) { - return result; + try { + if (auto result = it->second(descriptor)) { + return result; + } + log().error("Datasource constructor for type {} returned NULL.", type); + return nullptr; + } + catch (std::exception const& e) { + log().error("Exception while making `{}` datasource: {}", type, e.what()); + return nullptr; } - log().error("Datasource constructor for type {} returned NULL.", type); - return nullptr; } - log().error("No constructor registered for datasource type: {}", type); + log().error("No constructor is registered for datasource type: {}", type); return nullptr; } log().error("A YAML datasource descriptor is missing the `type` key!"); From f8863bc088e86c96be3b4f318f41d67c3fdce589 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 14:42:59 +0200 Subject: [PATCH 12/25] Additional armor for the config file watch thread. --- libs/service/src/config.cpp | 86 +++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 41 deletions(-) diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index 175a6a51..e73e4830 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -129,60 +129,64 @@ void DataSourceConfigService::restartFileWatchThread() watchThread_ = std::thread( [this, path = configFilePath_]() { - auto toStr = [](auto&& time){ + log().debug("Starting watch thread for {}.", path); + + auto toStr = [](fs::file_time_type const& time){ std::stringstream ss; - ss << time; + ss << std::chrono::duration_cast(time.time_since_epoch()).count(); return ss.str(); }; - log().debug("Starting watch thread for {}.", path); - - fs::file_time_type lastModTime; - bool fileExisted = fs::exists(path); + auto modTime = [](std::string const& path) -> std::optional { + try { + std::error_code e; + if (fs::exists(path)) { + auto result = fs::last_write_time(path, e); + if (!e) + return result; + } + } + catch (...) { + // Nothing to do. + } + return {}; + }; - if (fileExisted) { - lastModTime = fs::last_write_time(path); - log().debug("The config file exists (t={}).", toStr(lastModTime)); + auto lastModTime = modTime(path); + if (lastModTime) loadConfig(); - } - else { + else log().debug("The config file does not exist yet."); - } while (watching_) { std::this_thread::sleep_for(std::chrono::milliseconds(500)); - if (fs::exists(path)) { - if (!fileExisted) { - // The file has appeared since the last check. - lastModTime = fs::last_write_time(path); - log().debug("The config file exists now (t={}).", toStr(lastModTime)); - fileExisted = true; - loadConfig(); - } - else { - auto currentModTime = fs::last_write_time(path); - - if (currentModTime != lastModTime) { - // The file exists and has been modified since the last check. - log().debug( - "The config file changed ({} vs {}).", - toStr(currentModTime), - toStr(lastModTime)); - lastModTime = currentModTime; - loadConfig(); - } - else { - log().trace( - "The config file is unchanged ({} vs {}).", - toStr(currentModTime), - toStr(lastModTime)); - } - } + auto currentModTime = modTime(path); + + if (currentModTime && !lastModTime) { + // The file has appeared since the last check. + log().debug("The config file exists now (t={}).", toStr(*lastModTime)); + loadConfig(); } - else if (fileExisted) { + else if (!currentModTime && lastModTime) { log().debug("The config file disappeared."); - fileExisted = false; } + else if (currentModTime && lastModTime) { + if (*currentModTime != *lastModTime) { + // The file exists and has been modified since the last check. + log().debug( + "The config file changed (t0={} vs t1={}).", + toStr(*currentModTime), + toStr(*lastModTime)); + loadConfig(); + } + else + log().trace( + "The config file is unchanged (t0={} vs t1={}).", + toStr(*currentModTime), + toStr(*lastModTime)); + } + + lastModTime = currentModTime; } }); } From ea09eade421f7e6cb5ab50252ff38c688fb65196 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 15:15:51 +0200 Subject: [PATCH 13/25] Replace semaphore with atomic bool for cross-platform compatibility. --- test/unit/test-config.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 2fd3a96f..635e3225 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -2,7 +2,7 @@ #include #include #include -#include +#include #include "mapget/service/service.h" #include "mapget/service/datasource.h" @@ -31,10 +31,20 @@ std::string generateTimestampedDirectoryName(const std::string& baseName) { auto now = std::chrono::system_clock::now(); auto duration = now.time_since_epoch(); auto millis = std::chrono::duration_cast(duration).count(); - return baseName + "_" + std::to_string(millis); } +void waitForUpdate(std::atomic& flag, std::chrono::seconds timeout) { + auto start = std::chrono::steady_clock::now(); + while (!flag.load() && std::chrono::steady_clock::now() - start < timeout) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + if (!flag.load()) { + throw std::runtime_error("Timeout waiting for configuration update."); + } + flag.store(false); // Reset flag for next wait +} + TEST_CASE("Load Config From File", "[DataSourceConfig]") { setLogLevel("trace", log()); @@ -45,19 +55,18 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") DataSourceConfigService::get().registerDataSourceType( "TestDataSource", - [](const YAML::Node& config) -> DataSource::Ptr - { return std::make_shared(); }); + [](const YAML::Node& config) -> DataSource::Ptr { return std::make_shared(); }); auto cache = std::make_shared(); Service service(cache, true); REQUIRE(service.info().empty()); - std::binary_semaphore semaphore(0); + std::atomic updateOccurred(false); auto subscription = DataSourceConfigService::get().subscribe( [&](auto&&) { - log().debug("Release the semaphore!"); - semaphore.release(); + log().debug("Configuration update detected."); + updateOccurred.store(true); }); DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); @@ -67,9 +76,8 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; } - REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); + waitForUpdate(updateOccurred, std::chrono::seconds(5)); REQUIRE(service.info().empty()); - log().debug("Gate passed!"); // Adding a datasource std::this_thread::sleep_for(std::chrono::seconds(1)); @@ -80,9 +88,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") - type: TestDataSource )" << std::endl; } - REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); - log().debug("Gate passed!"); - + waitForUpdate(updateOccurred, std::chrono::seconds(5)); auto dataSourceInfos = service.info(); REQUIRE(dataSourceInfos.size() == 1); REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); @@ -93,9 +99,8 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []"; } - REQUIRE(semaphore.try_acquire_for(std::chrono::seconds(5))); + waitForUpdate(updateOccurred, std::chrono::seconds(5)); REQUIRE(service.info().empty()); - log().debug("Gate passed!"); // Cleanup fs::remove_all(tempDir); From 8e2f35d4b32e23a90aa5c1736342381a5ed15e34 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Fri, 12 Jul 2024 15:36:32 +0200 Subject: [PATCH 14/25] Fix sonar complaints. --- libs/http-service/src/cli.cpp | 6 +++--- libs/service/src/config.cpp | 8 ++++---- test/unit/test-config.cpp | 1 + 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index 6664ab53..92ff0741 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -100,8 +100,8 @@ class ConfigYAML : public CLI::Config { }; void registerDefaultDatasourceTypes() { - auto& config = DataSourceConfigService::get(); - config.registerDataSourceType( + auto& service = DataSourceConfigService::get(); + service.registerDataSourceType( "DataSourceHost", [](YAML::Node const& config) -> DataSource::Ptr { if (auto url = config["url"]) @@ -109,7 +109,7 @@ void registerDefaultDatasourceTypes() { else throw std::runtime_error("Missing `url` field."); }); - config.registerDataSourceType( + service.registerDataSourceType( "DataSourceProcess", [](YAML::Node const& config) -> DataSource::Ptr { if (auto cmd = config["cmd"]) diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index e73e4830..0591e1b5 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -68,7 +68,7 @@ void DataSourceConfigService::loadConfig() } } else { - log().warn("The config file {} does not have a sources node."); + log().debug("The config file {} does not have a sources node.", configFilePath_); } } catch (const YAML::Exception& e) { @@ -137,11 +137,11 @@ void DataSourceConfigService::restartFileWatchThread() return ss.str(); }; - auto modTime = [](std::string const& path) -> std::optional { + auto modTime = [](std::string const& checkPath) -> std::optional { try { std::error_code e; - if (fs::exists(path)) { - auto result = fs::last_write_time(path, e); + if (fs::exists(checkPath)) { + auto result = fs::last_write_time(checkPath, e); if (!e) return result; } diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 635e3225..426e44e9 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -72,6 +72,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); // Initial empty configuration + std::this_thread::sleep_for(std::chrono::seconds(1)); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; From 94b020538010f7e7db2aeb1f5fc92504ad6d201a Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Tue, 16 Jul 2024 19:50:18 +0200 Subject: [PATCH 15/25] Change some message log levels to debug. --- libs/service/src/service.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index 63371be8..b237e977 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -270,7 +270,7 @@ struct Service::Impl : public Service::Controller configSubscription_ = DataSourceConfigService::get().subscribe( [this](auto&& dataSourceConfigNodes) { - log().info("Config changed. Scanning for datasource changes..."); + log().debug("Config changed. Scanning for datasource changes..."); // Deserialize datasource configurations and update service accordingly std::map newConfigs; @@ -291,7 +291,7 @@ struct Service::Impl : public Service::Controller auto it = dataSourceConfigs_.begin(); while (it != dataSourceConfigs_.end()) { if (newConfigs.find(it->first) == newConfigs.end()) { - log().info("Removing datasource with config: {}", it->first); + log().debug("Removing datasource with config: {}", it->first); removeDataSource(it->second); it = dataSourceConfigs_.erase(it); } @@ -303,7 +303,7 @@ struct Service::Impl : public Service::Controller // Add or update datasources present in the new configuration for (const auto& [configKey, configNode] : newConfigs) { if (dataSourceConfigs_.find(configKey) == dataSourceConfigs_.end()) { - log().info("Adding new datasource with config: `{}`", configKey); + log().debug("Adding new datasource with config: `{}`", configKey); auto dataSource = DataSourceConfigService::get().makeDataSource(configNode); if (dataSource) { addDataSource(dataSource); @@ -316,7 +316,7 @@ struct Service::Impl : public Service::Controller } } else { - log().info( + log().debug( "Datasource already exists, no update required for config: {}", configKey); } From 5fc848944bb41876d38074aca9e8db7381ac54b4 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 08:05:16 +0200 Subject: [PATCH 16/25] Recreate all datasources when the config is changed. --- libs/service/src/service.cpp | 64 ++++++++++-------------------------- test/unit/test-config.cpp | 8 ++--- 2 files changed, 21 insertions(+), 51 deletions(-) diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index b237e977..9a57a39a 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -261,7 +261,7 @@ struct Service::Impl : public Service::Controller std::list addOnDataSources_; std::unique_ptr configSubscription_; - std::map dataSourceConfigs_; + std::vector dataSourcesFromConfig_; explicit Impl(Cache::Ptr cache, bool useDataSourceConfig) : Controller(std::move(cache)) { @@ -270,56 +270,26 @@ struct Service::Impl : public Service::Controller configSubscription_ = DataSourceConfigService::get().subscribe( [this](auto&& dataSourceConfigNodes) { - log().debug("Config changed. Scanning for datasource changes..."); - - // Deserialize datasource configurations and update service accordingly - std::map newConfigs; - for (const auto& node : dataSourceConfigNodes) { - YAML::Emitter out; - out << YAML::DoubleQuoted << YAML::Flow << node; - std::string serializedConfig = out.c_str(); - - if (!out.good()) { - log().error("YAML serialization failed: {}", out.GetLastError()); - continue; - } - - newConfigs[serializedConfig] = node; + // Remove previous datasources. + log().info("Config changed. Removing previous datasources."); + for (auto const& datasource : dataSourcesFromConfig_) { + removeDataSource(datasource); } - - // Remove datasources not present in the new configuration - auto it = dataSourceConfigs_.begin(); - while (it != dataSourceConfigs_.end()) { - if (newConfigs.find(it->first) == newConfigs.end()) { - log().debug("Removing datasource with config: {}", it->first); - removeDataSource(it->second); - it = dataSourceConfigs_.erase(it); - } - else { - ++it; - } - } - - // Add or update datasources present in the new configuration - for (const auto& [configKey, configNode] : newConfigs) { - if (dataSourceConfigs_.find(configKey) == dataSourceConfigs_.end()) { - log().debug("Adding new datasource with config: `{}`", configKey); - auto dataSource = DataSourceConfigService::get().makeDataSource(configNode); - if (dataSource) { - addDataSource(dataSource); - dataSourceConfigs_[configKey] = dataSource; - } - else { - log().error( - "Failed to makeDataSource datasource with config: {}", - configKey); - } + dataSourcesFromConfig_.clear(); + + // Add datasources present in the new configuration. + auto index = 0; + for (const auto& configNode : dataSourceConfigNodes) { + auto dataSource = DataSourceConfigService::get().makeDataSource(configNode); + if (dataSource) { + addDataSource(dataSource); + dataSourcesFromConfig_.push_back(dataSource); } else { - log().debug( - "Datasource already exists, no update required for config: {}", - configKey); + log().error( + "Failed to makeDataSource datasource at index {}.", index); } + ++index; } }); } diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 426e44e9..781b9b11 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -72,7 +72,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); // Initial empty configuration - std::this_thread::sleep_for(std::chrono::seconds(1)); + std::this_thread::sleep_for(std::chrono::seconds(2)); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; @@ -81,7 +81,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") REQUIRE(service.info().empty()); // Adding a datasource - std::this_thread::sleep_for(std::chrono::seconds(1)); + std::this_thread::sleep_for(std::chrono::seconds(2)); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << R"( @@ -95,7 +95,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); // Removing the datasource - std::this_thread::sleep_for(std::chrono::seconds(1)); + std::this_thread::sleep_for(std::chrono::seconds(2)); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []"; @@ -106,6 +106,6 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") // Cleanup fs::remove_all(tempDir); // Wait for "The config file disappeared" message :) - std::this_thread::sleep_for(std::chrono::seconds(1)); + std::this_thread::sleep_for(std::chrono::seconds(2)); DataSourceConfigService::get().end(); } From 1b0f085eea2b7520e30ecd07c6edfd2e4c1cc1d5 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 08:39:14 +0200 Subject: [PATCH 17/25] Update config docs. --- README.md | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index b8b04bca..33b4ef9e 100644 --- a/README.md +++ b/README.md @@ -33,12 +33,40 @@ mapget serve --help (or `python -m mapget --help` for the Python package). -The `mapget` executable can parse a config file with arguments supported by the command line interface. The path to the config file can be provided to `mapget` via command line by specifying the `--config` parameter. +The `mapget` executable can parse a `YAML`-based config file. The path to the config file can be provided to `mapget` via command line by specifying the `--config` parameter. +The config file may have two top-level keys which are used by mapget: The `sources` key and the `mapget` key. +### The `mapget` YAML key + +Under the `mapget` YAML key, any config options which can also be set via the command line may be set. +Note, that changes in this section are not applied while mapget is running, you will need to restart it. Sample configuration files can be found under `examples/config`: -- [sample-first-datasource.toml](examples/config/sample-first-datasource.yaml) and [sample-second-datasource.toml](examples/config/sample-second-datasource.yaml) will configure mapget to run a simple datasource with sample data. Note: the two formats in config files for subcommand parameters can be used interchangeably. -- [sample-service.toml](examples/config/sample-service.yaml) to execute the `mapget serve` command. The instance will fetch and serve data from sources started with `sample-*-datasource.toml` configs above. +- [sample-first-datasource.yaml](examples/config/sample-first-datasource.yaml) and [sample-second-datasource.yaml](examples/config/sample-second-datasource.yaml) will configure mapget to run a simple datasource with sample data. Note: the two formats in config files for subcommand parameters can be used interchangeably. +- [sample-service.yaml](examples/config/sample-service.yaml) to execute the `mapget serve` command. The instance will fetch and serve data from sources started with `sample-*-datasource.toml` configs above. + +### The `sources` YAML key + +Under the `sources` YAML key, you can configure datasources which are going to be served. +Note, that changes from the sources section are going to be applied immediately once the config +file is saved. This means, you can add and/or remove sources while mapget is running. +This section has the following format: The `sources` key must have a list. Each entry in the list +represents a datasource. The entry must have a `type` key, which denotes the specific datasource +constructor to call. You may register additional datasource types using the +`DatasourceConfigService` from `mapget/service/config.h`. By default, the following datasource types are supported: + +| Data Source Type | Required Configurations | Optional Configurations | +|-------------------------|-------------------------|-----------------------------| +| `DataSourceHost` | `url` | N/A | +| `DataSourceProcess` | `cmd` | N/A | + +For example, the following would be a valid configuration: + +```yaml +sources: + - type: DataSourceProcess + cmd: cpp-sample-http-datasource +``` ### Cache From 3540f2513e8b720ac00601f74712dc1c5555b5d8 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 11:56:09 +0200 Subject: [PATCH 18/25] Incorporate PR feedbaack. --- libs/http-service/src/cli.cpp | 55 ++++++++++++-------- libs/service/include/mapget/service/config.h | 6 +++ libs/service/src/config.cpp | 5 +- libs/service/src/service.cpp | 3 +- test/unit/test-config.cpp | 54 ++++++++++++------- 5 files changed, 79 insertions(+), 44 deletions(-) diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index 92ff0741..f2ad9271 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -18,10 +18,14 @@ namespace mapget namespace { -class ConfigYAML : public CLI::Config { +class ConfigYAML : public CLI::Config +{ public: - std::string to_config(const CLI::App *app, bool default_also, bool, std::string) const override { - std::string config_path = app->get_config_ptr() ? app->get_config_ptr()->as() : "config.yaml"; + std::string to_config(const CLI::App* app, bool defaultAlso, bool, std::string) const override + { + std::string config_path = app->get_config_ptr() ? + app->get_config_ptr()->as() : + "config.yaml"; std::ifstream ifs(config_path); YAML::Node root = ifs ? YAML::Load(ifs) : YAML::Node(); @@ -29,7 +33,7 @@ class ConfigYAML : public CLI::Config { auto mapgetNode = root["mapget"] = YAML::Node(YAML::NodeType::Map); // Process current app configuration into 'mapget' node - _to_yaml(mapgetNode, app, default_also); + toYaml(mapgetNode, app, defaultAlso); // Output the YAML content as a formatted string std::stringstream ss; @@ -37,8 +41,9 @@ class ConfigYAML : public CLI::Config { return ss.str(); } - void _to_yaml(YAML::Node root, const CLI::App *app, bool default_also) const { - for (const CLI::Option *opt : app->get_options({})) { + void toYaml(YAML::Node root, const CLI::App* app, bool defaultAlso) const + { + for (const CLI::Option* opt : app->get_options({})) { if (!opt->get_lnames().empty() && opt->get_configurable()) { std::string name = opt->get_lnames()[0]; @@ -47,49 +52,55 @@ class ConfigYAML : public CLI::Config { root[name] = opt->results().at(0); else if (opt->count() > 0) root[name] = opt->results(); - else if (default_also && !opt->get_default_str().empty()) + else if (defaultAlso && !opt->get_default_str().empty()) root[name] = opt->get_default_str(); } else if (opt->count()) { root[name] = opt->count() > 1 ? YAML::Node(opt->count()) : YAML::Node(true); } else { - root[name] = default_also ? YAML::Node(false) : YAML::Node(); + root[name] = defaultAlso ? YAML::Node(false) : YAML::Node(); } } } - for (const CLI::App *subcom : app->get_subcommands({})) - _to_yaml(root[subcom->get_name()], subcom, default_also); + for (const CLI::App* subcom : app->get_subcommands({})) + toYaml(root[subcom->get_name()], subcom, defaultAlso); } - std::vector from_config(std::istream &input) const override { + std::vector from_config(std::istream& input) const override + { YAML::Node root = YAML::Load(input); YAML::Node mapgetNode = root["mapget"]; - return mapgetNode ? _from_yaml(mapgetNode) : std::vector(); + return mapgetNode ? fromYaml(mapgetNode) : std::vector(); } - [[nodiscard]] std::vector _from_yaml(const YAML::Node &node, const std::string &name = "", const std::vector &prefix = {}) const { + [[nodiscard]] std::vector fromYaml( + const YAML::Node& node, + const std::string& name = "", + const std::vector& prefix = {}) const + { std::vector results; if (node.IsMap()) { - for (const auto &item : node) { + for (const auto& item : node) { auto copy_prefix = prefix; if (!name.empty()) { copy_prefix.push_back(name); } - auto sub_results = _from_yaml(item.second, item.first.as(), copy_prefix); + auto sub_results = fromYaml(item.second, item.first.as(), copy_prefix); results.insert(results.end(), sub_results.begin(), sub_results.end()); } - } else if (!name.empty()) { - results.emplace_back(); - CLI::ConfigItem &res = results.back(); + } + else if (!name.empty()) { + CLI::ConfigItem& res = results.emplace_back(); res.name = name; res.parents = prefix; if (node.IsScalar()) { res.inputs = {node.as()}; - } else if (node.IsSequence()) { - for (const auto &val : node) { + } + else if (node.IsSequence()) { + for (const auto& val : node) { res.inputs.push_back(val.as()); } } @@ -117,7 +128,7 @@ void registerDefaultDatasourceTypes() { else throw std::runtime_error("Missing `cmd` field."); }); -}; +} } @@ -192,7 +203,7 @@ struct ServeCommand watchConfig = true; registerDefaultDatasourceTypes(); DataSourceConfigService::get().setConfigFilePath(config->as()); - }; + } HttpService srv(cache, watchConfig); diff --git a/libs/service/include/mapget/service/config.h b/libs/service/include/mapget/service/config.h index 9818d4a0..faa98ff3 100644 --- a/libs/service/include/mapget/service/config.h +++ b/libs/service/include/mapget/service/config.h @@ -44,9 +44,15 @@ class DataSourceConfigService * Destructor that ensures unsubscription. */ ~Subscription(); + Subscription(Subscription const& other) = delete; + Subscription(Subscription&& other) = default; + Subscription& operator= (Subscription const& other) = delete; private: + explicit Subscription(uint32_t id); uint32_t id_ = 0; + + friend std::unique_ptr std::make_unique(uint32_t&&); friend class DataSourceConfigService; }; diff --git a/libs/service/src/config.cpp b/libs/service/src/config.cpp index 0591e1b5..db09bc00 100644 --- a/libs/service/src/config.cpp +++ b/libs/service/src/config.cpp @@ -22,6 +22,8 @@ DataSourceConfigService::Subscription::~Subscription() DataSourceConfigService::get().unsubscribe(id_); } +DataSourceConfigService::Subscription::Subscription(uint32_t id) : id_(id) {} + std::unique_ptr DataSourceConfigService::subscribe( std::function const&)> const& callback) { @@ -31,8 +33,7 @@ std::unique_ptr DataSourceConfigService:: } std::lock_guard memberAccessLock(memberAccessMutex_); - auto sub = std::make_unique(); - sub->id_ = nextSubscriptionId_++; + auto sub = std::make_unique(nextSubscriptionId_++); subscriptions_[sub->id_] = callback; // Optionally, trigger the callback with the current configuration immediately if (!currentConfig_.empty()) { diff --git a/libs/service/src/service.cpp b/libs/service/src/service.cpp index 9a57a39a..4d7048c9 100644 --- a/libs/service/src/service.cpp +++ b/libs/service/src/service.cpp @@ -280,8 +280,7 @@ struct Service::Impl : public Service::Controller // Add datasources present in the new configuration. auto index = 0; for (const auto& configNode : dataSourceConfigNodes) { - auto dataSource = DataSourceConfigService::get().makeDataSource(configNode); - if (dataSource) { + if (auto dataSource = DataSourceConfigService::get().makeDataSource(configNode)) { addDataSource(dataSource); dataSourcesFromConfig_.push_back(dataSource); } diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 781b9b11..717d0311 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -1,21 +1,24 @@ +#include #include +#include +#include #include #include -#include -#include +#include -#include "mapget/service/service.h" -#include "mapget/service/datasource.h" -#include "mapget/service/config.h" #include "mapget/log.h" +#include "mapget/service/config.h" +#include "mapget/service/datasource.h" #include "mapget/service/memcache.h" +#include "mapget/service/service.h" namespace fs = std::filesystem; using namespace mapget; struct TestDataSource : public DataSource { - DataSourceInfo info() override { + DataSourceInfo info() override + { return DataSourceInfo::fromJson(R"( { "mapId": "Catan", @@ -27,19 +30,26 @@ struct TestDataSource : public DataSource void fill(TileFeatureLayer::Ptr const& featureTile) override {}; }; -std::string generateTimestampedDirectoryName(const std::string& baseName) { +std::string generateTimestampedDirectoryName(const std::string& baseName) +{ auto now = std::chrono::system_clock::now(); auto duration = now.time_since_epoch(); auto millis = std::chrono::duration_cast(duration).count(); return baseName + "_" + std::to_string(millis); } -void waitForUpdate(std::atomic& flag, std::chrono::seconds timeout) { - auto start = std::chrono::steady_clock::now(); - while (!flag.load() && std::chrono::steady_clock::now() - start < timeout) { - std::this_thread::sleep_for(std::chrono::milliseconds(1)); +void waitForUpdate( + std::condition_variable& cv, + std::mutex& mtx, + std::atomic& flag, + std::chrono::seconds timeout) +{ + std::unique_lock lock(mtx); + if (flag.load()) { + flag.store(false); // Reset flag for next wait + return; } - if (!flag.load()) { + if (!cv.wait_for(lock, timeout, [&] { return flag.load(); })) { throw std::runtime_error("Timeout waiting for configuration update."); } flag.store(false); // Reset flag for next wait @@ -55,18 +65,26 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") DataSourceConfigService::get().registerDataSourceType( "TestDataSource", - [](const YAML::Node& config) -> DataSource::Ptr { return std::make_shared(); }); + [](const YAML::Node& config) -> DataSource::Ptr + { return std::make_shared(); }); auto cache = std::make_shared(); Service service(cache, true); REQUIRE(service.info().empty()); std::atomic updateOccurred(false); + std::condition_variable cv; + std::mutex mtx; auto subscription = DataSourceConfigService::get().subscribe( - [&](auto&&) { + [&](auto&&) + { log().debug("Configuration update detected."); - updateOccurred.store(true); + { + std::lock_guard lock(mtx); + updateOccurred.store(true); + } + cv.notify_all(); }); DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); @@ -77,7 +95,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; } - waitForUpdate(updateOccurred, std::chrono::seconds(5)); + waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); REQUIRE(service.info().empty()); // Adding a datasource @@ -89,7 +107,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") - type: TestDataSource )" << std::endl; } - waitForUpdate(updateOccurred, std::chrono::seconds(5)); + waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); auto dataSourceInfos = service.info(); REQUIRE(dataSourceInfos.size() == 1); REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); @@ -100,7 +118,7 @@ TEST_CASE("Load Config From File", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []"; } - waitForUpdate(updateOccurred, std::chrono::seconds(5)); + waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); REQUIRE(service.info().empty()); // Cleanup From 571bad8770e4c015c41a2bcf4abd6cc1a066d66e Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 14:12:06 +0200 Subject: [PATCH 19/25] Log config file load errors. --- libs/http-service/src/cli.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index f2ad9271..c16d3a53 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -70,9 +70,14 @@ class ConfigYAML : public CLI::Config std::vector from_config(std::istream& input) const override { - YAML::Node root = YAML::Load(input); - YAML::Node mapgetNode = root["mapget"]; - return mapgetNode ? fromYaml(mapgetNode) : std::vector(); + try { + YAML::Node root = YAML::Load(input); + YAML::Node mapgetNode = root["mapget"]; + return mapgetNode ? fromYaml(mapgetNode) : std::vector(); + } + catch (YAML::ParserException const& e) { + raise(fmt::format("Failed to parse config file! Error: {}", e.what())); + } } [[nodiscard]] std::vector fromYaml( From cc2d93338196f4f8625734c717fa324af06a1acd Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 15:02:19 +0200 Subject: [PATCH 20/25] Add test for config loading. --- .../include/mapget/http-service/cli.h | 2 +- libs/http-service/src/cli.cpp | 5 ++-- test/unit/test-config.cpp | 28 ++++++++++++++++++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/libs/http-service/include/mapget/http-service/cli.h b/libs/http-service/include/mapget/http-service/cli.h index 2aa608be..00edae02 100644 --- a/libs/http-service/include/mapget/http-service/cli.h +++ b/libs/http-service/include/mapget/http-service/cli.h @@ -4,5 +4,5 @@ #include namespace mapget { - int runFromCommandLine(std::vector args); + int runFromCommandLine(std::vector args, bool requireSubcommand = true); } diff --git a/libs/http-service/src/cli.cpp b/libs/http-service/src/cli.cpp index c16d3a53..bc9787da 100644 --- a/libs/http-service/src/cli.cpp +++ b/libs/http-service/src/cli.cpp @@ -314,7 +314,7 @@ struct FetchCommand } }; -int runFromCommandLine(std::vector args) +int runFromCommandLine(std::vector args, bool requireSubcommand) { CLI::App app{"A client/server application for map data retrieval."}; std::string log_level_; @@ -329,7 +329,8 @@ int runFromCommandLine(std::vector args) "Optional path to a file with configuration arguments for mapget."); app.config_formatter(std::make_shared()); - app.require_subcommand(1); + if (requireSubcommand) + app.require_subcommand(1); if (!log_level_.empty()) { mapget::setLogLevel(log_level_, log()); diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 717d0311..c4e8ffcc 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -55,7 +55,33 @@ void waitForUpdate( flag.store(false); // Reset flag for next wait } -TEST_CASE("Load Config From File", "[DataSourceConfig]") +TEST_CASE("Mapget Config", "[MapgetConfig]") +{ + auto tempDir = fs::temp_directory_path() / generateTimestampedDirectoryName("mapget_test"); + fs::create_directory(tempDir); + auto tempConfigPath = tempDir / "temp_config.yaml"; + + SECTION("Bad Config") + { + std::ofstream out(tempConfigPath, std::ios_base::trunc); + out << "sources: [" << std::endl; + REQUIRE(mapget::runFromCommandLine({ + "--config", tempConfigPath + }) == 1); + } + + SECTION("Good Config") + { + std::ofstream out(tempConfigPath, std::ios_base::trunc); + out << R"( + sources: + - type: TestDataSource + )" << std::endl; + REQUIRE(mapget::runFromCommandLine({"--config", tempConfigPath}, false) == 0); + } +} + +TEST_CASE("Datasource Config", "[DataSourceConfig]") { setLogLevel("trace", log()); From 628e78a3ac2b80e19a4b090cd37b3902e3625cc7 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 15:03:25 +0200 Subject: [PATCH 21/25] Use promise/future in test. --- test/unit/test-config.cpp | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index c4e8ffcc..11061689 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -1,16 +1,16 @@ #include #include #include -#include #include #include -#include +#include #include "mapget/log.h" #include "mapget/service/config.h" #include "mapget/service/datasource.h" #include "mapget/service/memcache.h" #include "mapget/service/service.h" +#include "mapget/http-service/cli.h" namespace fs = std::filesystem; using namespace mapget; @@ -38,21 +38,11 @@ std::string generateTimestampedDirectoryName(const std::string& baseName) return baseName + "_" + std::to_string(millis); } -void waitForUpdate( - std::condition_variable& cv, - std::mutex& mtx, - std::atomic& flag, - std::chrono::seconds timeout) +void waitForUpdate(std::future& future, std::chrono::seconds timeout) { - std::unique_lock lock(mtx); - if (flag.load()) { - flag.store(false); // Reset flag for next wait - return; - } - if (!cv.wait_for(lock, timeout, [&] { return flag.load(); })) { + if (future.wait_for(timeout) != std::future_status::ready) { throw std::runtime_error("Timeout waiting for configuration update."); } - flag.store(false); // Reset flag for next wait } TEST_CASE("Mapget Config", "[MapgetConfig]") @@ -98,19 +88,14 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") Service service(cache, true); REQUIRE(service.info().empty()); - std::atomic updateOccurred(false); - std::condition_variable cv; - std::mutex mtx; + std::promise updatePromise; + auto updateFuture = updatePromise.get_future(); auto subscription = DataSourceConfigService::get().subscribe( [&](auto&&) { log().debug("Configuration update detected."); - { - std::lock_guard lock(mtx); - updateOccurred.store(true); - } - cv.notify_all(); + updatePromise.set_value(); }); DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); @@ -121,9 +106,13 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; } - waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); + waitForUpdate(updateFuture, std::chrono::seconds(5)); REQUIRE(service.info().empty()); + // Reset future for next update + updatePromise = std::promise(); + updateFuture = updatePromise.get_future(); + // Adding a datasource std::this_thread::sleep_for(std::chrono::seconds(2)); { @@ -133,7 +122,7 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") - type: TestDataSource )" << std::endl; } - waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); + waitForUpdate(updateFuture, std::chrono::seconds(5)); auto dataSourceInfos = service.info(); REQUIRE(dataSourceInfos.size() == 1); REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); @@ -144,7 +133,7 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []"; } - waitForUpdate(cv, mtx, updateOccurred, std::chrono::seconds(5)); + waitForUpdate(updateFuture, std::chrono::seconds(5)); REQUIRE(service.info().empty()); // Cleanup From a49604983e1ca0275b763029571a043aa8a079e1 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 15:17:57 +0200 Subject: [PATCH 22/25] Fix config test. --- test/unit/test-config.cpp | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 11061689..8fb3d0dd 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -5,12 +5,12 @@ #include #include +#include "mapget/http-service/cli.h" #include "mapget/log.h" #include "mapget/service/config.h" #include "mapget/service/datasource.h" #include "mapget/service/memcache.h" #include "mapget/service/service.h" -#include "mapget/http-service/cli.h" namespace fs = std::filesystem; using namespace mapget; @@ -38,9 +38,9 @@ std::string generateTimestampedDirectoryName(const std::string& baseName) return baseName + "_" + std::to_string(millis); } -void waitForUpdate(std::future& future, std::chrono::seconds timeout) +void waitForUpdate(std::future& future) { - if (future.wait_for(timeout) != std::future_status::ready) { + if (future.wait_for(std::chrono::seconds(5)) != std::future_status::ready) { throw std::runtime_error("Timeout waiting for configuration update."); } } @@ -55,9 +55,7 @@ TEST_CASE("Mapget Config", "[MapgetConfig]") { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: [" << std::endl; - REQUIRE(mapget::runFromCommandLine({ - "--config", tempConfigPath - }) == 1); + REQUIRE(mapget::runFromCommandLine({"--config", tempConfigPath}) == 1); } SECTION("Good Config") @@ -90,6 +88,12 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") std::promise updatePromise; auto updateFuture = updatePromise.get_future(); + auto prepareNextUpdate = [&]() + { + std::this_thread::sleep_for(std::chrono::seconds(2)); + updatePromise = std::promise(); + updateFuture = updatePromise.get_future(); + }; auto subscription = DataSourceConfigService::get().subscribe( [&](auto&&) @@ -101,20 +105,16 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") DataSourceConfigService::get().setConfigFilePath(tempConfigPath.string()); // Initial empty configuration - std::this_thread::sleep_for(std::chrono::seconds(2)); + prepareNextUpdate(); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: []" << std::endl; } - waitForUpdate(updateFuture, std::chrono::seconds(5)); + waitForUpdate(updateFuture); REQUIRE(service.info().empty()); - // Reset future for next update - updatePromise = std::promise(); - updateFuture = updatePromise.get_future(); - // Adding a datasource - std::this_thread::sleep_for(std::chrono::seconds(2)); + prepareNextUpdate(); { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << R"( @@ -122,18 +122,18 @@ TEST_CASE("Datasource Config", "[DataSourceConfig]") - type: TestDataSource )" << std::endl; } - waitForUpdate(updateFuture, std::chrono::seconds(5)); + waitForUpdate(updateFuture); auto dataSourceInfos = service.info(); REQUIRE(dataSourceInfos.size() == 1); REQUIRE(dataSourceInfos[0].mapId_ == "Catan"); // Removing the datasource - std::this_thread::sleep_for(std::chrono::seconds(2)); + prepareNextUpdate(); { std::ofstream out(tempConfigPath, std::ios_base::trunc); - out << "sources: []"; + out << "sources: []" << std::endl; } - waitForUpdate(updateFuture, std::chrono::seconds(5)); + waitForUpdate(updateFuture); REQUIRE(service.info().empty()); // Cleanup From e16fb292474b9902c150647f22bb381a7d2a5ea6 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 16:12:36 +0200 Subject: [PATCH 23/25] Pass missing flag in mapget.run() --- libs/pymapget/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/pymapget/__main__.py b/libs/pymapget/__main__.py index 8b14d5f8..6da3e4fa 100644 --- a/libs/pymapget/__main__.py +++ b/libs/pymapget/__main__.py @@ -3,5 +3,5 @@ import time if __name__ == "__main__": - ret_code = mapget.run(sys.argv[1:]) + ret_code = mapget.run(sys.argv[1:], True) exit(ret_code) From f2f723e1a732f3620058f29ea36b220c6c908afe Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 16:26:10 +0200 Subject: [PATCH 24/25] Fix string init list. --- test/unit/test-config.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index 8fb3d0dd..a2d3ae25 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -55,7 +55,7 @@ TEST_CASE("Mapget Config", "[MapgetConfig]") { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: [" << std::endl; - REQUIRE(mapget::runFromCommandLine({"--config", tempConfigPath}) == 1); + REQUIRE(mapget::runFromCommandLine(std::vector{std::string("--config"), tempConfigPath}) == 1); } SECTION("Good Config") @@ -65,7 +65,7 @@ TEST_CASE("Mapget Config", "[MapgetConfig]") sources: - type: TestDataSource )" << std::endl; - REQUIRE(mapget::runFromCommandLine({"--config", tempConfigPath}, false) == 0); + REQUIRE(mapget::runFromCommandLine(std::vector{std::string("--config"), tempConfigPath}, false) == 0); } } From 0dc2805c35f973d1fefb019c754ad2de5c88e225 Mon Sep 17 00:00:00 2001 From: Joseph Birkner Date: Thu, 18 Jul 2024 17:33:34 +0200 Subject: [PATCH 25/25] Eat this, MSVC! --- test/unit/test-config.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/test-config.cpp b/test/unit/test-config.cpp index a2d3ae25..ee717c32 100644 --- a/test/unit/test-config.cpp +++ b/test/unit/test-config.cpp @@ -55,7 +55,7 @@ TEST_CASE("Mapget Config", "[MapgetConfig]") { std::ofstream out(tempConfigPath, std::ios_base::trunc); out << "sources: [" << std::endl; - REQUIRE(mapget::runFromCommandLine(std::vector{std::string("--config"), tempConfigPath}) == 1); + REQUIRE(mapget::runFromCommandLine({std::string("--config"), tempConfigPath.string()}) == 1); } SECTION("Good Config") @@ -65,7 +65,7 @@ TEST_CASE("Mapget Config", "[MapgetConfig]") sources: - type: TestDataSource )" << std::endl; - REQUIRE(mapget::runFromCommandLine(std::vector{std::string("--config"), tempConfigPath}, false) == 0); + REQUIRE(mapget::runFromCommandLine({std::string("--config"), tempConfigPath.string()}, false) == 0); } }