From 36a9f40a60b6a2e9ab89660219e8700b481a82cc Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 7 Jan 2025 14:52:56 +0000 Subject: [PATCH 01/31] fix: Optimize `ledger_range` query (#1797) --- src/data/cassandra/Schema.hpp | 7 ++++--- tests/integration/data/cassandra/BaseTests.cpp | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/data/cassandra/Schema.hpp b/src/data/cassandra/Schema.hpp index 9816c53bd..4ec0356f9 100644 --- a/src/data/cassandra/Schema.hpp +++ b/src/data/cassandra/Schema.hpp @@ -74,7 +74,7 @@ public: 'class': 'SimpleStrategy', 'replication_factor': '{}' }} - AND durable_writes = true + AND durable_writes = True )", settingsProvider_.get().getKeyspace(), settingsProvider_.get().getReplicationFactor() @@ -472,7 +472,7 @@ public: R"( UPDATE {} SET sequence = ? - WHERE is_latest = false + WHERE is_latest = False )", qualifiedTableName(settingsProvider_.get(), "ledger_range") )); @@ -776,7 +776,7 @@ public: R"( SELECT sequence FROM {} - WHERE is_latest = true + WHERE is_latest = True )", qualifiedTableName(settingsProvider_.get(), "ledger_range") )); @@ -787,6 +787,7 @@ public: R"( SELECT sequence FROM {} + WHERE is_latest in (True, False) )", qualifiedTableName(settingsProvider_.get(), "ledger_range") )); diff --git a/tests/integration/data/cassandra/BaseTests.cpp b/tests/integration/data/cassandra/BaseTests.cpp index 7f9c86c6b..c1b4dcd2d 100644 --- a/tests/integration/data/cassandra/BaseTests.cpp +++ b/tests/integration/data/cassandra/BaseTests.cpp @@ -52,7 +52,7 @@ protected: R"( CREATE KEYSPACE IF NOT EXISTS {} WITH replication = {{'class': 'SimpleStrategy', 'replication_factor': '1'}} - AND durable_writes = true + AND durable_writes = True )", keyspace ); @@ -211,7 +211,7 @@ TEST_F(BackendCassandraBaseTest, KeyspaceManipulation) R"( CREATE KEYSPACE {} WITH replication = {{'class': 'SimpleStrategy', 'replication_factor': '1'}} - AND durable_writes = true + AND durable_writes = True )", keyspace ); From 48c8d85d0c6ca72bdf5020eef6fbd4eba87b8b66 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Thu, 9 Jan 2025 14:47:08 +0000 Subject: [PATCH 02/31] feat: ETLng loader basics (#1808) For #1597 --- src/etl/ETLService.cpp | 2 +- src/etl/impl/AmendmentBlockHandler.cpp | 2 +- src/etl/impl/AmendmentBlockHandler.hpp | 2 +- src/etl/impl/Transformer.hpp | 2 +- src/etlng/AmendmentBlockHandlerInterface.hpp | 37 ++++ src/etlng/CMakeLists.txt | 5 +- src/etlng/LoadBalancerInterface.hpp | 134 ++++++++++++++ src/etlng/LoaderInterface.hpp | 52 ++++++ src/etlng/Models.hpp | 47 +++++ src/etlng/impl/AmendmentBlockHandler.cpp | 56 ++++++ src/etlng/impl/AmendmentBlockHandler.hpp | 69 ++++++++ src/etlng/impl/AsyncGrpcCall.cpp | 6 +- src/etlng/impl/AsyncGrpcCall.hpp | 2 +- src/etlng/impl/GrpcSource.cpp | 4 +- src/etlng/impl/Loading.cpp | 119 +++++++++++++ src/etlng/impl/Loading.hpp | 85 +++++++++ src/util/Repeat.cpp | 1 + src/util/Repeat.hpp | 19 +- src/util/async/AnyStopToken.hpp | 1 + src/util/async/context/impl/Cancellation.hpp | 2 + tests/common/util/BinaryTestObject.cpp | 2 +- .../common/util/MockAmendmentBlockHandler.hpp | 6 +- .../common/util/MockETLServiceTestFixture.hpp | 29 +++ tests/common/util/MockLedgerFetcher.hpp | 6 + tests/common/util/MockLoadBalancer.hpp | 38 ++++ tests/unit/CMakeLists.txt | 2 + tests/unit/etl/AmendmentBlockHandlerTests.cpp | 4 +- .../unit/etlng/AmendmentBlockHandlerTests.cpp | 69 ++++++++ tests/unit/etlng/ExtractionTests.cpp | 165 ++++++++++++++++-- tests/unit/etlng/LoadingTests.cpp | 160 +++++++++++++++++ tests/unit/util/BytesConverterTests.cpp | 1 + 31 files changed, 1093 insertions(+), 36 deletions(-) create mode 100644 src/etlng/AmendmentBlockHandlerInterface.hpp create mode 100644 src/etlng/LoadBalancerInterface.hpp create mode 100644 src/etlng/LoaderInterface.hpp create mode 100644 src/etlng/impl/AmendmentBlockHandler.cpp create mode 100644 src/etlng/impl/AmendmentBlockHandler.hpp create mode 100644 src/etlng/impl/Loading.cpp create mode 100644 src/etlng/impl/Loading.hpp create mode 100644 tests/unit/etlng/AmendmentBlockHandlerTests.cpp create mode 100644 tests/unit/etlng/LoadingTests.cpp diff --git a/src/etl/ETLService.cpp b/src/etl/ETLService.cpp index 3807efed8..2b35a96ac 100644 --- a/src/etl/ETLService.cpp +++ b/src/etl/ETLService.cpp @@ -134,7 +134,7 @@ ETLService::monitor() } } catch (std::runtime_error const& e) { LOG(log_.fatal()) << "Failed to load initial ledger: " << e.what(); - amendmentBlockHandler_.onAmendmentBlock(); + amendmentBlockHandler_.notifyAmendmentBlocked(); return; } diff --git a/src/etl/impl/AmendmentBlockHandler.cpp b/src/etl/impl/AmendmentBlockHandler.cpp index 9b732bf6b..d4899f807 100644 --- a/src/etl/impl/AmendmentBlockHandler.cpp +++ b/src/etl/impl/AmendmentBlockHandler.cpp @@ -47,7 +47,7 @@ AmendmentBlockHandler::AmendmentBlockHandler( } void -AmendmentBlockHandler::onAmendmentBlock() +AmendmentBlockHandler::notifyAmendmentBlocked() { state_.get().isAmendmentBlocked = true; repeat_.start(interval_, action_); diff --git a/src/etl/impl/AmendmentBlockHandler.hpp b/src/etl/impl/AmendmentBlockHandler.hpp index b50d909b2..a49e75cb7 100644 --- a/src/etl/impl/AmendmentBlockHandler.hpp +++ b/src/etl/impl/AmendmentBlockHandler.hpp @@ -53,7 +53,7 @@ public: ); void - onAmendmentBlock(); + notifyAmendmentBlocked(); }; } // namespace etl::impl diff --git a/src/etl/impl/Transformer.hpp b/src/etl/impl/Transformer.hpp index 834c3130d..b1d33b054 100644 --- a/src/etl/impl/Transformer.hpp +++ b/src/etl/impl/Transformer.hpp @@ -203,7 +203,7 @@ private: } catch (std::runtime_error const& e) { LOG(log_.fatal()) << "Failed to build next ledger: " << e.what(); - amendmentBlockHandler_.get().onAmendmentBlock(); + amendmentBlockHandler_.get().notifyAmendmentBlocked(); return {ripple::LedgerHeader{}, false}; } diff --git a/src/etlng/AmendmentBlockHandlerInterface.hpp b/src/etlng/AmendmentBlockHandlerInterface.hpp new file mode 100644 index 000000000..985d0e9cb --- /dev/null +++ b/src/etlng/AmendmentBlockHandlerInterface.hpp @@ -0,0 +1,37 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +namespace etlng { + +/** + * @brief The interface of a handler for amendment blocking + */ +struct AmendmentBlockHandlerInterface { + virtual ~AmendmentBlockHandlerInterface() = default; + + /** + * @brief The function to call once an amendment block has been discovered + */ + virtual void + notifyAmendmentBlocked() = 0; +}; + +} // namespace etlng diff --git a/src/etlng/CMakeLists.txt b/src/etlng/CMakeLists.txt index 5d9e29158..3ffc81317 100644 --- a/src/etlng/CMakeLists.txt +++ b/src/etlng/CMakeLists.txt @@ -1,5 +1,8 @@ add_library(clio_etlng) -target_sources(clio_etlng PRIVATE impl/AsyncGrpcCall.cpp impl/Extraction.cpp impl/GrpcSource.cpp) +target_sources( + clio_etlng PRIVATE impl/AmendmentBlockHandler.cpp impl/AsyncGrpcCall.cpp impl/Extraction.cpp impl/GrpcSource.cpp + impl/Loading.cpp +) target_link_libraries(clio_etlng PUBLIC clio_data) diff --git a/src/etlng/LoadBalancerInterface.hpp b/src/etlng/LoadBalancerInterface.hpp new file mode 100644 index 000000000..3466a6e79 --- /dev/null +++ b/src/etlng/LoadBalancerInterface.hpp @@ -0,0 +1,134 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +/** @file */ +#pragma once + +#include "etl/ETLState.hpp" +#include "etlng/InitialLoadObserverInterface.hpp" +#include "rpc/Errors.hpp" + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace etlng { + +/** + * @brief An interface for LoadBalancer + */ +class LoadBalancerInterface { +public: + using RawLedgerObjectType = org::xrpl::rpc::v1::RawLedgerObject; + using GetLedgerResponseType = org::xrpl::rpc::v1::GetLedgerResponse; + using OptionalGetLedgerResponseType = std::optional; + + virtual ~LoadBalancerInterface() = default; + + /** + * @brief Load the initial ledger, writing data to the queue. + * @note This function will retry indefinitely until the ledger is downloaded. + * + * @param sequence Sequence of ledger to download + * @param loader InitialLoadObserverInterface implementation + * @param retryAfter Time to wait between retries (2 seconds by default) + * @return A std::vector The ledger data + */ + virtual std::vector + loadInitialLedger( + uint32_t sequence, + etlng::InitialLoadObserverInterface& loader, + std::chrono::steady_clock::duration retryAfter = std::chrono::seconds{2} + ) = 0; + + /** + * @brief Load the initial ledger, writing data to the queue. + * @note This function will retry indefinitely until the ledger is downloaded. + * + * @param sequence Sequence of ledger to download + * @param retryAfter Time to wait between retries (2 seconds by default) + * @return A std::vector The ledger data + */ + virtual std::vector + loadInitialLedger(uint32_t sequence, std::chrono::steady_clock::duration retryAfter = std::chrono::seconds{2}) = 0; + + /** + * @brief Fetch data for a specific ledger. + * + * This function will continuously try to fetch data for the specified ledger until the fetch succeeds, the ledger + * is found in the database, or the server is shutting down. + * + * @param ledgerSequence Sequence of the ledger to fetch + * @param getObjects Whether to get the account state diff between this ledger and the prior one + * @param getObjectNeighbors Whether to request object neighbors + * @param retryAfter Time to wait between retries (2 seconds by default) + * @return The extracted data, if extraction was successful. If the ledger was found + * in the database or the server is shutting down, the optional will be empty + */ + virtual OptionalGetLedgerResponseType + fetchLedger( + uint32_t ledgerSequence, + bool getObjects, + bool getObjectNeighbors, + std::chrono::steady_clock::duration retryAfter = std::chrono::seconds{2} + ) = 0; + + /** + * @brief Represent the state of this load balancer as a JSON object + * + * @return JSON representation of the state of this load balancer. + */ + virtual boost::json::value + toJson() const = 0; + + /** + * @brief Forward a JSON RPC request to a randomly selected rippled node. + * + * @param request JSON-RPC request to forward + * @param clientIp The IP address of the peer, if known + * @param isAdmin Whether the request is from an admin + * @param yield The coroutine context + * @return Response received from rippled node as JSON object on success or error on failure + */ + virtual std::expected + forwardToRippled( + boost::json::object const& request, + std::optional const& clientIp, + bool isAdmin, + boost::asio::yield_context yield + ) = 0; + + /** + * @brief Return state of ETL nodes. + * @return ETL state, nullopt if etl nodes not available + */ + virtual std::optional + getETLState() noexcept = 0; +}; + +} // namespace etlng diff --git a/src/etlng/LoaderInterface.hpp b/src/etlng/LoaderInterface.hpp new file mode 100644 index 000000000..929d71679 --- /dev/null +++ b/src/etlng/LoaderInterface.hpp @@ -0,0 +1,52 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "etlng/Models.hpp" + +#include + +#include + +namespace etlng { + +/** + * @brief An interface for a ETL Loader + */ +struct LoaderInterface { + virtual ~LoaderInterface() = default; + + /** + * @brief Load ledger data + * @param data The data to load + */ + virtual void + load(model::LedgerData const& data) = 0; + + /** + * @brief Load the initial ledger + * @param data The data to load + * @return Optional ledger header + */ + virtual std::optional + loadInitialLedger(model::LedgerData const& data) = 0; +}; + +} // namespace etlng diff --git a/src/etlng/Models.hpp b/src/etlng/Models.hpp index 4e8aa6825..afe6238b0 100644 --- a/src/etlng/Models.hpp +++ b/src/etlng/Models.hpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -79,6 +80,23 @@ struct Transaction { ripple::uint256 id; std::string key; // key is the above id as a string of 32 characters ripple::TxType type; + + /** + * @brief Compares Transaction objects to each other without considering sttx and meta fields + * @param other The Transaction to compare to + * @return true if transaction is equivalent; false otherwise + */ + bool + operator==(Transaction const& other) const + { + return raw == other.raw // + and metaRaw == other.metaRaw // + and sttx.getTransactionID() == other.sttx.getTransactionID() // + and meta.getTxID() == other.meta.getTxID() // + and id == other.id // + and key == other.key // + and type == other.type; + } }; /** @@ -103,6 +121,9 @@ struct Object { std::string predecessor; ModType type; + + bool + operator==(Object const&) const = default; }; /** @@ -111,6 +132,9 @@ struct Object { struct BookSuccessor { std::string firstBook; std::string bookBase; + + bool + operator==(BookSuccessor const&) const = default; }; /** @@ -125,6 +149,29 @@ struct LedgerData { ripple::LedgerHeader header; std::string rawHeader; uint32_t seq; + + /** + * @brief Compares LedgerData objects to each other without considering the header field + * @param other The LedgerData to compare to + * @return true if data is equivalent; false otherwise + */ + bool + operator==(LedgerData const& other) const + { + auto const serialized = [](auto const& hdr) { + ripple::Serializer ser; + ripple::addRaw(hdr, ser); + return ser.getString(); + }; + + return transactions == other.transactions // + and objects == other.objects // + and successors == other.successors // + and edgeKeys == other.edgeKeys // + and serialized(header) == serialized(other.header) // + and rawHeader == other.rawHeader // + and seq == other.seq; + } }; } // namespace etlng::model diff --git a/src/etlng/impl/AmendmentBlockHandler.cpp b/src/etlng/impl/AmendmentBlockHandler.cpp new file mode 100644 index 000000000..502e31812 --- /dev/null +++ b/src/etlng/impl/AmendmentBlockHandler.cpp @@ -0,0 +1,56 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "etlng/impl/AmendmentBlockHandler.hpp" + +#include "etl/SystemState.hpp" +#include "util/async/AnyExecutionContext.hpp" +#include "util/log/Logger.hpp" + +#include +#include +#include + +namespace etlng::impl { + +AmendmentBlockHandler::ActionType const AmendmentBlockHandler::kDEFAULT_AMENDMENT_BLOCK_ACTION = []() { + static util::Logger const log{"ETL"}; // NOLINT(readability-identifier-naming) + LOG(log.fatal()) << "Can't process new ledgers: The current ETL source is not compatible with the version of " + << "the libxrpl Clio is currently using. Please upgrade Clio to a newer version."; +}; + +AmendmentBlockHandler::AmendmentBlockHandler( + util::async::AnyExecutionContext&& ctx, + etl::SystemState& state, + std::chrono::steady_clock::duration interval, + ActionType action +) + : state_{std::ref(state)}, interval_{interval}, ctx_{std::move(ctx)}, action_{std::move(action)} +{ +} + +void +AmendmentBlockHandler::notifyAmendmentBlocked() +{ + state_.get().isAmendmentBlocked = true; + if (not operation_.has_value()) + operation_.emplace(ctx_.executeRepeatedly(interval_, action_)); +} + +} // namespace etlng::impl diff --git a/src/etlng/impl/AmendmentBlockHandler.hpp b/src/etlng/impl/AmendmentBlockHandler.hpp new file mode 100644 index 000000000..0bece8e79 --- /dev/null +++ b/src/etlng/impl/AmendmentBlockHandler.hpp @@ -0,0 +1,69 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "etl/SystemState.hpp" +#include "etlng/AmendmentBlockHandlerInterface.hpp" +#include "util/async/AnyExecutionContext.hpp" +#include "util/async/AnyOperation.hpp" + +#include +#include +#include + +#include +#include +#include + +namespace etlng::impl { + +class AmendmentBlockHandler : public AmendmentBlockHandlerInterface { +public: + using ActionType = std::function; + +private: + std::reference_wrapper state_; + std::chrono::steady_clock::duration interval_; + util::async::AnyExecutionContext ctx_; + std::optional> operation_; + + ActionType action_; + +public: + static ActionType const kDEFAULT_AMENDMENT_BLOCK_ACTION; + + AmendmentBlockHandler( + util::async::AnyExecutionContext&& ctx, + etl::SystemState& state, + std::chrono::steady_clock::duration interval = std::chrono::seconds{1}, + ActionType action = kDEFAULT_AMENDMENT_BLOCK_ACTION + ); + + ~AmendmentBlockHandler() override + { + if (operation_.has_value()) + operation_.value().abort(); + } + + void + notifyAmendmentBlocked() override; +}; + +} // namespace etlng::impl diff --git a/src/etlng/impl/AsyncGrpcCall.cpp b/src/etlng/impl/AsyncGrpcCall.cpp index 6fd44dced..87e132503 100644 --- a/src/etlng/impl/AsyncGrpcCall.cpp +++ b/src/etlng/impl/AsyncGrpcCall.cpp @@ -87,14 +87,14 @@ AsyncGrpcCall::process( if (abort) { LOG(log_.error()) << "AsyncGrpcCall aborted"; - return CallStatus::ERRORED; + return CallStatus::Errored; } if (!status_.ok()) { LOG(log_.error()) << "AsyncGrpcCall status_ not ok: code = " << status_.error_code() << " message = " << status_.error_message(); - return CallStatus::ERRORED; + return CallStatus::Errored; } if (!next_->is_unlimited()) { @@ -141,7 +141,7 @@ AsyncGrpcCall::process( predecessorKey_ = lastKey_; // but for ongoing onInitialObjects calls we need to pass along the key we left // off at so that we can link the two lists correctly - return more ? CallStatus::MORE : CallStatus::DONE; + return more ? CallStatus::More : CallStatus::Done; } void diff --git a/src/etlng/impl/AsyncGrpcCall.hpp b/src/etlng/impl/AsyncGrpcCall.hpp index 8cb3edb84..044323257 100644 --- a/src/etlng/impl/AsyncGrpcCall.hpp +++ b/src/etlng/impl/AsyncGrpcCall.hpp @@ -38,7 +38,7 @@ namespace etlng::impl { class AsyncGrpcCall { public: - enum class CallStatus { MORE, DONE, ERRORED }; + enum class CallStatus { More, Done, Errored }; using RequestType = org::xrpl::rpc::v1::GetLedgerDataRequest; using ResponseType = org::xrpl::rpc::v1::GetLedgerDataResponse; using StubType = org::xrpl::rpc::v1::XRPLedgerAPIService::Stub; diff --git a/src/etlng/impl/GrpcSource.cpp b/src/etlng/impl/GrpcSource.cpp index adcbba7e7..a537bcd08 100644 --- a/src/etlng/impl/GrpcSource.cpp +++ b/src/etlng/impl/GrpcSource.cpp @@ -139,7 +139,7 @@ GrpcSource::loadInitialLedger( LOG(log_.trace()) << "Marker prefix = " << ptr->getMarkerPrefix(); auto result = ptr->process(stub_, queue, observer, abort); - if (result != AsyncGrpcCall::CallStatus::MORE) { + if (result != AsyncGrpcCall::CallStatus::More) { ++numFinished; LOG(log_.debug()) << "Finished a marker. Current number of finished = " << numFinished; @@ -147,7 +147,7 @@ GrpcSource::loadInitialLedger( edgeKeys.push_back(std::move(lastKey)); } - if (result == AsyncGrpcCall::CallStatus::ERRORED) + if (result == AsyncGrpcCall::CallStatus::Errored) abort = true; } diff --git a/src/etlng/impl/Loading.cpp b/src/etlng/impl/Loading.cpp new file mode 100644 index 000000000..c35841e04 --- /dev/null +++ b/src/etlng/impl/Loading.cpp @@ -0,0 +1,119 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "etlng/impl/Loading.hpp" + +#include "data/BackendInterface.hpp" +#include "etl/LedgerFetcherInterface.hpp" +#include "etl/impl/LedgerLoader.hpp" +#include "etlng/AmendmentBlockHandlerInterface.hpp" +#include "etlng/Models.hpp" +#include "etlng/RegistryInterface.hpp" +#include "util/Assert.hpp" +#include "util/LedgerUtils.hpp" +#include "util/Profiler.hpp" +#include "util/log/Logger.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace etlng::impl { + +Loader::Loader( + std::shared_ptr backend, + std::shared_ptr fetcher, + std::shared_ptr registry, + std::shared_ptr amendmentBlockHandler +) + : backend_(std::move(backend)) + , fetcher_(std::move(fetcher)) + , registry_(std::move(registry)) + , amendmentBlockHandler_(std::move(amendmentBlockHandler)) +{ +} + +void +Loader::load(model::LedgerData const& data) +{ + try { + // perform cache updates and all writes from extensions + registry_->dispatch(data); + + auto [success, duration] = + ::util::timed>([&]() { return backend_->finishWrites(data.seq); }); + LOG(log_.info()) << "Finished writes to DB for " << data.seq << ": " << (success ? "YES" : "NO") << "; took " + << duration; + } catch (std::runtime_error const& e) { + LOG(log_.fatal()) << "Failed to load " << data.seq << ": " << e.what(); + amendmentBlockHandler_->notifyAmendmentBlocked(); + } +}; + +void +Loader::onInitialLoadGotMoreObjects( + uint32_t seq, + std::vector const& data, + std::optional lastKey +) +{ + LOG(log_.debug()) << "On initial load: got more objects for seq " << seq << ". size = " << data.size(); + registry_->dispatchInitialObjects( + seq, data, std::move(lastKey).value_or(std::string{}) // TODO: perhaps use optional all the way to extensions? + ); +} + +std::optional +Loader::loadInitialLedger(model::LedgerData const& data) +{ + // check that database is actually empty + auto rng = backend_->hardFetchLedgerRangeNoThrow(); + if (rng) { + ASSERT(false, "Database is not empty"); + return std::nullopt; + } + + LOG(log_.debug()) << "Deserialized ledger header. " << ::util::toString(data.header); + + auto seconds = ::util::timed([this, &data]() { registry_->dispatchInitialData(data); }); + LOG(log_.info()) << "Dispatching initial data and submitting all writes took " << seconds << " seconds."; + + backend_->finishWrites(data.seq); + LOG(log_.debug()) << "Loaded initial ledger"; + + return {data.header}; +} + +} // namespace etlng::impl diff --git a/src/etlng/impl/Loading.hpp b/src/etlng/impl/Loading.hpp new file mode 100644 index 000000000..435dcb4e4 --- /dev/null +++ b/src/etlng/impl/Loading.hpp @@ -0,0 +1,85 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "data/BackendInterface.hpp" +#include "etl/LedgerFetcherInterface.hpp" +#include "etl/impl/LedgerLoader.hpp" +#include "etlng/AmendmentBlockHandlerInterface.hpp" +#include "etlng/InitialLoadObserverInterface.hpp" +#include "etlng/LoaderInterface.hpp" +#include "etlng/Models.hpp" +#include "etlng/RegistryInterface.hpp" +#include "util/log/Logger.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace etlng::impl { + +class Loader : public LoaderInterface, public InitialLoadObserverInterface { + std::shared_ptr backend_; + std::shared_ptr fetcher_; + std::shared_ptr registry_; + std::shared_ptr amendmentBlockHandler_; + + util::Logger log_{"ETL"}; + +public: + using RawLedgerObjectType = org::xrpl::rpc::v1::RawLedgerObject; + using GetLedgerResponseType = org::xrpl::rpc::v1::GetLedgerResponse; + using OptionalGetLedgerResponseType = std::optional; + + Loader( + std::shared_ptr backend, + std::shared_ptr fetcher, + std::shared_ptr registry, + std::shared_ptr amendmentBlockHandler + ); + + void + load(model::LedgerData const& data) override; + + void + onInitialLoadGotMoreObjects( + uint32_t seq, + std::vector const& data, + std::optional lastKey + ) override; + + std::optional + loadInitialLedger(model::LedgerData const& data) override; +}; + +} // namespace etlng::impl diff --git a/src/util/Repeat.cpp b/src/util/Repeat.cpp index 9b55b9fd1..725f008a0 100644 --- a/src/util/Repeat.cpp +++ b/src/util/Repeat.cpp @@ -26,6 +26,7 @@ Repeat::stop() { if (control_->stopping) return; + control_->stopping = true; control_->timer.cancel(); control_->semaphore.acquire(); diff --git a/src/util/Repeat.hpp b/src/util/Repeat.hpp index 79730c419..1746f87d7 100644 --- a/src/util/Repeat.hpp +++ b/src/util/Repeat.hpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace util { @@ -48,7 +49,7 @@ class Repeat { } }; - std::unique_ptr control_; + std::shared_ptr control_; public: /** @@ -89,23 +90,23 @@ public: { ASSERT(control_->stopping, "Should be stopped before starting"); control_->stopping = false; - startImpl(interval, std::forward(action)); + startImpl(control_, interval, std::forward(action)); } private: template - void - startImpl(std::chrono::steady_clock::duration interval, Action&& action) + static void + startImpl(std::shared_ptr control, std::chrono::steady_clock::duration interval, Action&& action) { - control_->timer.expires_after(interval); - control_->timer.async_wait([this, interval, action = std::forward(action)](auto const& ec) mutable { - if (ec or control_->stopping) { - control_->semaphore.release(); + control->timer.expires_after(interval); + control->timer.async_wait([control, interval, action = std::forward(action)](auto const& ec) mutable { + if (ec or control->stopping) { + control->semaphore.release(); return; } action(); - startImpl(interval, std::forward(action)); + startImpl(std::move(control), interval, std::forward(action)); }); } }; diff --git a/src/util/async/AnyStopToken.hpp b/src/util/async/AnyStopToken.hpp index 9ce36f3d7..6370ede63 100644 --- a/src/util/async/AnyStopToken.hpp +++ b/src/util/async/AnyStopToken.hpp @@ -24,6 +24,7 @@ #include +#include #include #include #include diff --git a/src/util/async/context/impl/Cancellation.hpp b/src/util/async/context/impl/Cancellation.hpp index d29fa5be5..9dfd57d7e 100644 --- a/src/util/async/context/impl/Cancellation.hpp +++ b/src/util/async/context/impl/Cancellation.hpp @@ -19,8 +19,10 @@ #pragma once +#include #include #include +#include #include #include diff --git a/tests/common/util/BinaryTestObject.cpp b/tests/common/util/BinaryTestObject.cpp index b25c0f599..b9082e7d0 100644 --- a/tests/common/util/BinaryTestObject.cpp +++ b/tests/common/util/BinaryTestObject.cpp @@ -211,7 +211,7 @@ createObject() .dataRaw = hexStringToBinaryString(kOBJ_BLOB), .successor = hexStringToBinaryString(kOBJ_SUCC), .predecessor = hexStringToBinaryString(kOBJ_PRED), - .type = {}, + .type = etlng::model::Object::ModType::Created, }; } diff --git a/tests/common/util/MockAmendmentBlockHandler.hpp b/tests/common/util/MockAmendmentBlockHandler.hpp index 08df2f0b3..6f1b14444 100644 --- a/tests/common/util/MockAmendmentBlockHandler.hpp +++ b/tests/common/util/MockAmendmentBlockHandler.hpp @@ -19,8 +19,10 @@ #pragma once +#include "etlng/AmendmentBlockHandlerInterface.hpp" + #include -struct MockAmendmentBlockHandler { - MOCK_METHOD(void, onAmendmentBlock, (), ()); +struct MockAmendmentBlockHandler : etlng::AmendmentBlockHandlerInterface { + MOCK_METHOD(void, notifyAmendmentBlocked, (), (override)); }; diff --git a/tests/common/util/MockETLServiceTestFixture.hpp b/tests/common/util/MockETLServiceTestFixture.hpp index a15f7f060..9009ab1fe 100644 --- a/tests/common/util/MockETLServiceTestFixture.hpp +++ b/tests/common/util/MockETLServiceTestFixture.hpp @@ -20,11 +20,15 @@ #pragma once #include "util/LoggerFixtures.hpp" +#include "util/MockAmendmentBlockHandler.hpp" #include "util/MockETLService.hpp" +#include "util/MockLedgerFetcher.hpp" #include "util/MockLoadBalancer.hpp" #include +#include + /** * @brief Fixture with a mock etl service */ @@ -63,3 +67,28 @@ struct MockLoadBalancerTest : virtual public NoLoggerFixture { protected: std::shared_ptr mockLoadBalancerPtr_ = std::make_shared(); }; + +/** + * @brief Fixture with a mock NG etl balancer + */ +struct MockNgLoadBalancerTest : virtual public NoLoggerFixture { +protected: + std::shared_ptr mockLoadBalancerPtr_ = std::make_shared(); +}; + +/** + * @brief Fixture with a mock ledger fetcher + */ +struct MockLedgerFetcherTest : virtual public NoLoggerFixture { +protected: + std::shared_ptr mockLedgerFetcherPtr_ = std::make_shared(); +}; + +/** + * @brief Fixture with a mock ledger fetcher + */ +struct MockAmendmentBlockHandlerTest : virtual public NoLoggerFixture { +protected: + std::shared_ptr mockAmendmentBlockHandlerPtr_ = + std::make_shared(); +}; diff --git a/tests/common/util/MockLedgerFetcher.hpp b/tests/common/util/MockLedgerFetcher.hpp index 405306533..5c73e6b71 100644 --- a/tests/common/util/MockLedgerFetcher.hpp +++ b/tests/common/util/MockLedgerFetcher.hpp @@ -19,6 +19,7 @@ #pragma once +#include "etl/LedgerFetcherInterface.hpp" #include "util/FakeFetchResponse.hpp" #include @@ -30,3 +31,8 @@ struct MockLedgerFetcher { MOCK_METHOD(std::optional, fetchData, (uint32_t), ()); MOCK_METHOD(std::optional, fetchDataAndDiff, (uint32_t), ()); }; + +struct MockNgLedgerFetcher : etl::LedgerFetcherInterface { + MOCK_METHOD(OptionalGetLedgerResponseType, fetchData, (uint32_t), (override)); + MOCK_METHOD(OptionalGetLedgerResponseType, fetchDataAndDiff, (uint32_t), (override)); +}; diff --git a/tests/common/util/MockLoadBalancer.hpp b/tests/common/util/MockLoadBalancer.hpp index c51e25db1..12bcfea78 100644 --- a/tests/common/util/MockLoadBalancer.hpp +++ b/tests/common/util/MockLoadBalancer.hpp @@ -19,6 +19,9 @@ #pragma once +#include "etl/ETLState.hpp" +#include "etlng/InitialLoadObserverInterface.hpp" +#include "etlng/LoadBalancerInterface.hpp" #include "rpc/Errors.hpp" #include "util/FakeFetchResponse.hpp" @@ -28,10 +31,12 @@ #include #include +#include #include #include #include #include +#include struct MockLoadBalancer { using RawLedgerObjectType = FakeLedgerObject; @@ -48,3 +53,36 @@ struct MockLoadBalancer { (const) ); }; + +struct MockNgLoadBalancer : etlng::LoadBalancerInterface { + using RawLedgerObjectType = FakeLedgerObject; + + MOCK_METHOD( + std::vector, + loadInitialLedger, + (uint32_t, etlng::InitialLoadObserverInterface&, std::chrono::steady_clock::duration), + (override) + ); + MOCK_METHOD( + std::vector, + loadInitialLedger, + (uint32_t, std::chrono::steady_clock::duration), + (override) + ); + MOCK_METHOD( + OptionalGetLedgerResponseType, + fetchLedger, + (uint32_t, bool, bool, std::chrono::steady_clock::duration), + (override) + ); + MOCK_METHOD(boost::json::value, toJson, (), (const, override)); + MOCK_METHOD(std::optional, getETLState, (), (noexcept, override)); + + using ForwardToRippledReturnType = std::expected; + MOCK_METHOD( + ForwardToRippledReturnType, + forwardToRippled, + (boost::json::object const&, std::optional const&, bool, boost::asio::yield_context), + (override) + ); +}; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 589c78019..f1eda9b2b 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -33,9 +33,11 @@ target_sources( etl/SubscriptionSourceTests.cpp etl/TransformerTests.cpp # ETLng + etlng/AmendmentBlockHandlerTests.cpp etlng/ExtractionTests.cpp etlng/GrpcSourceTests.cpp etlng/RegistryTests.cpp + etlng/LoadingTests.cpp # Feed util/BytesConverterTests.cpp feed/BookChangesFeedTests.cpp diff --git a/tests/unit/etl/AmendmentBlockHandlerTests.cpp b/tests/unit/etl/AmendmentBlockHandlerTests.cpp index eadf48245..bfc4ff319 100644 --- a/tests/unit/etl/AmendmentBlockHandlerTests.cpp +++ b/tests/unit/etl/AmendmentBlockHandlerTests.cpp @@ -36,13 +36,13 @@ struct AmendmentBlockHandlerTest : util::prometheus::WithPrometheus, SyncAsioCon etl::SystemState state; }; -TEST_F(AmendmentBlockHandlerTest, CallToOnAmendmentBlockSetsStateAndRepeatedlyCallsAction) +TEST_F(AmendmentBlockHandlerTest, CallTonotifyAmendmentBlockedSetsStateAndRepeatedlyCallsAction) { AmendmentBlockHandler handler{ctx_, state, std::chrono::nanoseconds{1}, actionMock.AsStdFunction()}; EXPECT_FALSE(state.isAmendmentBlocked); EXPECT_CALL(actionMock, Call()).Times(testing::AtLeast(10)); - handler.onAmendmentBlock(); + handler.notifyAmendmentBlocked(); EXPECT_TRUE(state.isAmendmentBlocked); runContextFor(std::chrono::milliseconds{1}); diff --git a/tests/unit/etlng/AmendmentBlockHandlerTests.cpp b/tests/unit/etlng/AmendmentBlockHandlerTests.cpp new file mode 100644 index 000000000..13813ec45 --- /dev/null +++ b/tests/unit/etlng/AmendmentBlockHandlerTests.cpp @@ -0,0 +1,69 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "etl/SystemState.hpp" +#include "etlng/impl/AmendmentBlockHandler.hpp" +#include "util/LoggerFixtures.hpp" +#include "util/MockPrometheus.hpp" +#include "util/async/context/BasicExecutionContext.hpp" + +#include +#include + +#include +#include +#include + +using namespace etlng::impl; + +struct AmendmentBlockHandlerNgTests : util::prometheus::WithPrometheus { +protected: + testing::StrictMock> actionMock_; + etl::SystemState state_; + + util::async::CoroExecutionContext ctx_; +}; + +TEST_F(AmendmentBlockHandlerNgTests, CallTonotifyAmendmentBlockedSetsStateAndRepeatedlyCallsAction) +{ + constexpr static auto kMAX_ITERATIONS = 10uz; + etlng::impl::AmendmentBlockHandler handler{ctx_, state_, std::chrono::nanoseconds{1}, actionMock_.AsStdFunction()}; + auto counter = 0uz; + std::binary_semaphore stop{0}; + + EXPECT_FALSE(state_.isAmendmentBlocked); + EXPECT_CALL(actionMock_, Call()).Times(testing::AtLeast(10)).WillRepeatedly([&]() { + if (++counter; counter > kMAX_ITERATIONS) + stop.release(); + }); + + handler.notifyAmendmentBlocked(); + stop.acquire(); // wait for the counter to reach over kMAX_ITERATIONS + + EXPECT_TRUE(state_.isAmendmentBlocked); +} + +struct DefaultAmendmentBlockActionNgTest : LoggerFixture {}; + +TEST_F(DefaultAmendmentBlockActionNgTest, Call) +{ + AmendmentBlockHandler::kDEFAULT_AMENDMENT_BLOCK_ACTION(); + auto const loggerString = getLoggerString(); + EXPECT_TRUE(loggerString.starts_with("ETL:FTL Can't process new ledgers")) << "LoggerString " << loggerString; +} diff --git a/tests/unit/etlng/ExtractionTests.cpp b/tests/unit/etlng/ExtractionTests.cpp index 4aa213ba9..012145bc9 100644 --- a/tests/unit/etlng/ExtractionTests.cpp +++ b/tests/unit/etlng/ExtractionTests.cpp @@ -24,10 +24,12 @@ #include "etlng/impl/Extraction.hpp" #include "util/BinaryTestObject.hpp" #include "util/LoggerFixtures.hpp" +#include "util/TestObject.hpp" #include #include #include +#include #include #include #include @@ -38,14 +40,146 @@ #include #include #include +#include +#include namespace { +constinit auto const kLEDGER_HASH = "4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25E4AAB854A6A652"; +constinit auto const kLEDGER_HASH2 = "1B8590C01B0006EDFA9ED60296DD052DC5E90F99659B25014D08E1BC983515BC"; constinit auto const kSEQ = 30; } // namespace -struct ExtractionTests : NoLoggerFixture {}; +struct ExtractionModelNgTests : NoLoggerFixture {}; -TEST_F(ExtractionTests, ModType) +TEST_F(ExtractionModelNgTests, LedgerDataCopyableAndEquatable) +{ + auto const first = etlng::model::LedgerData{ + .transactions = + {util::createTransaction(ripple::TxType::ttNFTOKEN_BURN), + util::createTransaction(ripple::TxType::ttNFTOKEN_BURN), + util::createTransaction(ripple::TxType::ttNFTOKEN_CREATE_OFFER)}, + .objects = {util::createObject(), util::createObject(), util::createObject()}, + .successors = std::vector{{.firstBook = "first", .bookBase = "base"}}, + .edgeKeys = std::vector{"key1", "key2"}, + .header = createLedgerHeader(kLEDGER_HASH, kSEQ, 1), + .rawHeader = {1, 2, 3}, + .seq = kSEQ + }; + + auto const second = first; + EXPECT_EQ(first, second); + + { + auto third = second; + third.transactions.clear(); + EXPECT_NE(first, third); + } + { + auto third = second; + third.objects = {util::createObject()}; + EXPECT_NE(first, third); + } + { + auto third = second; + third.successors = std::vector{{.firstBook = "second", .bookBase = "base"}}; + EXPECT_NE(first, third); + } + { + auto third = second; + third.edgeKeys = std::vector{"key1"}; + EXPECT_NE(first, third); + } + { + auto third = second; + third.header = createLedgerHeader(kLEDGER_HASH2, kSEQ, 2); + EXPECT_NE(first, third); + } + { + auto third = second; + third.rawHeader = {2, 3, 4}; + EXPECT_NE(first, third); + } + { + auto third = second; + third.seq = kSEQ - 1; + EXPECT_NE(first, third); + } +} + +TEST_F(ExtractionModelNgTests, TransactionIsEquatable) +{ + auto const tx = std::vector{util::createTransaction(ripple::TxType::ttNFTOKEN_BURN)}; + auto other = tx; + EXPECT_EQ(tx, other); + + other.push_back(util::createTransaction(ripple::TxType::ttNFTOKEN_ACCEPT_OFFER)); + EXPECT_NE(tx, other); +} + +TEST_F(ExtractionModelNgTests, ObjectCopyableAndEquatable) +{ + auto const obj = util::createObject(); + auto const other = obj; + EXPECT_EQ(obj, other); + + { + auto third = other; + third.key = ripple::uint256{42}; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.keyRaw = "key"; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.data = {2, 3}; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.dataRaw = "something"; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.successor = "succ"; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.predecessor = "pred"; + EXPECT_NE(obj, third); + } + { + auto third = other; + third.type = etlng::model::Object::ModType::Deleted; + EXPECT_NE(obj, third); + } +} + +TEST_F(ExtractionModelNgTests, BookSuccessorCopyableAndEquatable) +{ + auto const succ = etlng::model::BookSuccessor{.firstBook = "first", .bookBase = "base"}; + auto const other = succ; + EXPECT_EQ(succ, other); + + { + auto third = other; + third.bookBase = "all your base are belong to us"; + EXPECT_NE(succ, third); + } + { + auto third = other; + third.firstBook = "not the first book"; + EXPECT_NE(succ, third); + } +} + +struct ExtractionNgTests : NoLoggerFixture {}; + +TEST_F(ExtractionNgTests, ModType) { using namespace etlng::impl; using ModType = etlng::model::Object::ModType; @@ -56,7 +190,7 @@ TEST_F(ExtractionTests, ModType) EXPECT_EQ(extractModType(PBObjType::UNSPECIFIED), ModType::Unspecified); } -TEST_F(ExtractionTests, OneTransaction) +TEST_F(ExtractionNgTests, OneTransaction) { using namespace etlng::impl; @@ -74,7 +208,7 @@ TEST_F(ExtractionTests, OneTransaction) EXPECT_EQ(res.sttx.getTxnType(), expected.sttx.getTxnType()); } -TEST_F(ExtractionTests, MultipleTransactions) +TEST_F(ExtractionNgTests, MultipleTransactions) { using namespace etlng::impl; @@ -102,7 +236,7 @@ TEST_F(ExtractionTests, MultipleTransactions) } } -TEST_F(ExtractionTests, OneObject) +TEST_F(ExtractionNgTests, OneObject) { using namespace etlng::impl; @@ -110,6 +244,9 @@ TEST_F(ExtractionTests, OneObject) auto original = org::xrpl::rpc::v1::RawLedgerObject(); original.set_data(expected.dataRaw); original.set_key(expected.keyRaw); + original.set_mod_type( + org::xrpl::rpc::v1::RawLedgerObject::ModificationType::RawLedgerObject_ModificationType_CREATED + ); auto res = extractObj(original); EXPECT_EQ(ripple::strHex(res.key), ripple::strHex(expected.keyRaw)); @@ -119,7 +256,7 @@ TEST_F(ExtractionTests, OneObject) EXPECT_EQ(res.type, expected.type); } -TEST_F(ExtractionTests, OneObjectWithSuccessorAndPredecessor) +TEST_F(ExtractionNgTests, OneObjectWithSuccessorAndPredecessor) { using namespace etlng::impl; @@ -129,6 +266,9 @@ TEST_F(ExtractionTests, OneObjectWithSuccessorAndPredecessor) original.set_key(expected.keyRaw); original.set_predecessor(expected.predecessor); original.set_successor(expected.successor); + original.set_mod_type( + org::xrpl::rpc::v1::RawLedgerObject::ModificationType::RawLedgerObject_ModificationType_CREATED + ); auto res = extractObj(original); EXPECT_EQ(ripple::strHex(res.key), ripple::strHex(expected.keyRaw)); @@ -138,7 +278,7 @@ TEST_F(ExtractionTests, OneObjectWithSuccessorAndPredecessor) EXPECT_EQ(res.type, expected.type); } -TEST_F(ExtractionTests, MultipleObjects) +TEST_F(ExtractionNgTests, MultipleObjects) { using namespace etlng::impl; @@ -146,6 +286,9 @@ TEST_F(ExtractionTests, MultipleObjects) auto original = org::xrpl::rpc::v1::RawLedgerObject(); original.set_data(expected.dataRaw); original.set_key(expected.keyRaw); + original.set_mod_type( + org::xrpl::rpc::v1::RawLedgerObject::ModificationType::RawLedgerObject_ModificationType_CREATED + ); auto list = org::xrpl::rpc::v1::RawLedgerObjects(); for (auto i = 0; i < 10; ++i) { @@ -165,7 +308,7 @@ TEST_F(ExtractionTests, MultipleObjects) } } -TEST_F(ExtractionTests, OneSuccessor) +TEST_F(ExtractionNgTests, OneSuccessor) { using namespace etlng::impl; @@ -179,7 +322,7 @@ TEST_F(ExtractionTests, OneSuccessor) EXPECT_EQ(ripple::strHex(res.bookBase), ripple::strHex(expected.bookBase)); } -TEST_F(ExtractionTests, MultipleSuccessors) +TEST_F(ExtractionNgTests, MultipleSuccessors) { using namespace etlng::impl; @@ -205,7 +348,7 @@ TEST_F(ExtractionTests, MultipleSuccessors) } } -TEST_F(ExtractionTests, SuccessorsWithNoNeighborsIncluded) +TEST_F(ExtractionNgTests, SuccessorsWithNoNeighborsIncluded) { using namespace etlng::impl; @@ -238,7 +381,7 @@ struct MockFetcher : etl::LedgerFetcherInterface { MOCK_METHOD(std::optional, fetchDataAndDiff, (uint32_t), (override)); }; -struct ExtractorTests : ExtractionTests { +struct ExtractorTests : ExtractionNgTests { std::shared_ptr fetcher = std::make_shared(); etlng::impl::Extractor extractor{fetcher}; }; diff --git a/tests/unit/etlng/LoadingTests.cpp b/tests/unit/etlng/LoadingTests.cpp new file mode 100644 index 000000000..5f14513bf --- /dev/null +++ b/tests/unit/etlng/LoadingTests.cpp @@ -0,0 +1,160 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "data/Types.hpp" +#include "etlng/InitialLoadObserverInterface.hpp" +#include "etlng/Models.hpp" +#include "etlng/RegistryInterface.hpp" +#include "etlng/impl/Loading.hpp" +#include "rpc/RPCHelpers.hpp" +#include "util/BinaryTestObject.hpp" +#include "util/MockBackendTestFixture.hpp" +#include "util/MockETLServiceTestFixture.hpp" +#include "util/MockPrometheus.hpp" +#include "util/TestObject.hpp" + +#include +#include + +#include +#include +#include +#include +#include +#include + +using namespace etlng::model; +using namespace etlng::impl; + +namespace { + +constinit auto const kLEDGER_HASH = "4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25E4AAB854A6A652"; +constinit auto const kSEQ = 30; + +struct MockRegistry : etlng::RegistryInterface { + MOCK_METHOD(void, dispatchInitialObjects, (uint32_t, std::vector const&, std::string), (override)); + MOCK_METHOD(void, dispatchInitialData, (LedgerData const&), (override)); + MOCK_METHOD(void, dispatch, (LedgerData const&), (override)); +}; + +struct MockLoadObserver : etlng::InitialLoadObserverInterface { + MOCK_METHOD( + void, + onInitialLoadGotMoreObjects, + (uint32_t, std::vector const&, std::optional), + (override) + ); +}; + +struct LoadingTests : util::prometheus::WithPrometheus, + MockBackendTest, + MockLedgerFetcherTest, + MockAmendmentBlockHandlerTest { +protected: + std::shared_ptr mockRegistryPtr_ = std::make_shared(); + Loader loader_{backend_, mockLedgerFetcherPtr_, mockRegistryPtr_, mockAmendmentBlockHandlerPtr_}; +}; + +struct LoadingDeathTest : LoadingTests {}; + +auto +createTestData() +{ + auto const header = createLedgerHeader(kLEDGER_HASH, kSEQ); + return LedgerData{ + .transactions = {}, + .objects = {util::createObject(), util::createObject(), util::createObject()}, + .successors = {}, + .edgeKeys = {}, + .header = header, + .rawHeader = {}, + .seq = kSEQ + }; +} + +} // namespace + +TEST_F(LoadingTests, LoadInitialLedger) +{ + auto const data = createTestData(); + + EXPECT_CALL(*backend_, hardFetchLedgerRange(testing::_)).WillOnce(testing::Return(std::nullopt)); + EXPECT_CALL(*backend_, doFinishWrites()); + EXPECT_CALL(*mockRegistryPtr_, dispatchInitialData(data)); + + auto const res = loader_.loadInitialLedger(data); + EXPECT_TRUE(res.has_value()); + EXPECT_EQ(rpc::ledgerHeaderToBlob(res.value(), true), rpc::ledgerHeaderToBlob(data.header, true)); +} + +TEST_F(LoadingTests, LoadSuccess) +{ + auto const data = createTestData(); + + EXPECT_CALL(*backend_, doFinishWrites()); + EXPECT_CALL(*mockRegistryPtr_, dispatch(data)); + + loader_.load(data); +} + +TEST_F(LoadingTests, LoadFailure) +{ + auto const data = createTestData(); + + EXPECT_CALL(*backend_, doFinishWrites()).Times(0); + EXPECT_CALL(*mockRegistryPtr_, dispatch(data)).WillOnce([](auto const&) { + throw std::runtime_error("some error"); + }); + EXPECT_CALL(*mockAmendmentBlockHandlerPtr_, notifyAmendmentBlocked()); + + loader_.load(data); +} + +TEST_F(LoadingTests, OnInitialLoadGotMoreObjectsWithKey) +{ + auto const data = createTestData(); + auto const lastKey = std::make_optional("something"); + + EXPECT_CALL(*mockRegistryPtr_, dispatchInitialObjects(kSEQ, data.objects, lastKey->data())); + + loader_.onInitialLoadGotMoreObjects(kSEQ, data.objects, lastKey); +} + +TEST_F(LoadingTests, OnInitialLoadGotMoreObjectsWithoutKey) +{ + auto const data = createTestData(); + auto const lastKey = std::optional{}; + + EXPECT_CALL(*mockRegistryPtr_, dispatchInitialObjects(kSEQ, data.objects, std::string{})); + + loader_.onInitialLoadGotMoreObjects(kSEQ, data.objects, lastKey); +} + +TEST_F(LoadingDeathTest, LoadInitialLedgerHasDataInDB) +{ + auto const data = createTestData(); + auto const range = LedgerRange{.minSequence = kSEQ - 1, .maxSequence = kSEQ}; + + // backend_ leaks due to death test. would be nice to figure out a better solution but for now + // we simply don't set expectations and allow the mock to leak + testing::Mock::AllowLeak(&*backend_); + ON_CALL(*backend_, hardFetchLedgerRange(testing::_)).WillByDefault(testing::Return(range)); + + EXPECT_DEATH({ [[maybe_unused]] auto unused = loader_.loadInitialLedger(data); }, ".*"); +} diff --git a/tests/unit/util/BytesConverterTests.cpp b/tests/unit/util/BytesConverterTests.cpp index 5396e4583..01841c0c0 100644 --- a/tests/unit/util/BytesConverterTests.cpp +++ b/tests/unit/util/BytesConverterTests.cpp @@ -20,6 +20,7 @@ #include "util/BytesConverter.hpp" #include + #include #include From 590c07ad8473f497130806f51717c81bfacc294c Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Thu, 9 Jan 2025 15:26:25 +0000 Subject: [PATCH 03/31] fix: AsyncFramework RAII (#1815) Fixes #1812 --- src/util/MoveTracker.hpp | 78 +++++++++++++++++++ src/util/async/Operation.hpp | 42 ++++++++-- tests/unit/CMakeLists.txt | 6 +- tests/unit/util/MoveTrackerTests.cpp | 68 ++++++++++++++++ .../util/async/AsyncExecutionContextTests.cpp | 70 ++++++++++++++++- 5 files changed, 255 insertions(+), 9 deletions(-) create mode 100644 src/util/MoveTracker.hpp create mode 100644 tests/unit/util/MoveTrackerTests.cpp diff --git a/src/util/MoveTracker.hpp b/src/util/MoveTracker.hpp new file mode 100644 index 000000000..22d0bd8ae --- /dev/null +++ b/src/util/MoveTracker.hpp @@ -0,0 +1,78 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include + +namespace util { + +/** + * @brief A base-class that can be used to check whether the current instance was moved from + */ +class MoveTracker { + bool wasMoved_ = false; + +protected: + /** + * @brief The function to be used by clients in order to check whether the instance was moved from + * @return true if moved from; false otherwise + */ + [[nodiscard]] bool + wasMoved() const noexcept + { + return wasMoved_; + } + + MoveTracker() = default; // should only be used via inheritance + +public: + virtual ~MoveTracker() = default; + + /** + * @brief Move constructor sets the moved-from state on `other` and resets the state on `this` + * @param other The moved-from object + */ + MoveTracker(MoveTracker&& other) + { + *this = std::move(other); + } + + /** + * @brief Move operator sets the moved-from state on `other` and resets the state on `this` + * @param other The moved-from object + * @return Reference to self + */ + MoveTracker& + operator=(MoveTracker&& other) + { + if (this != &other) { + other.wasMoved_ = true; + wasMoved_ = false; + } + + return *this; + } + + MoveTracker(MoveTracker const&) = default; + MoveTracker& + operator=(MoveTracker const&) = default; +}; + +} // namespace util diff --git a/src/util/async/Operation.hpp b/src/util/async/Operation.hpp index 742e346cd..1ad0c41fe 100644 --- a/src/util/async/Operation.hpp +++ b/src/util/async/Operation.hpp @@ -19,9 +19,9 @@ #pragma once +#include "util/MoveTracker.hpp" #include "util/Repeat.hpp" #include "util/async/Concepts.hpp" -#include "util/async/Error.hpp" #include "util/async/Outcome.hpp" #include "util/async/context/impl/Cancellation.hpp" #include "util/async/context/impl/Timer.hpp" @@ -36,7 +36,6 @@ #include #include #include -#include namespace util::async { namespace impl { @@ -71,7 +70,7 @@ public: }; template -struct BasicScheduledOperation { +struct BasicScheduledOperation : util::MoveTracker { class State { std::mutex m_; std::condition_variable ready_; @@ -105,6 +104,19 @@ struct BasicScheduledOperation { { } + ~BasicScheduledOperation() + { + if (not wasMoved()) + abort(); + } + + BasicScheduledOperation(BasicScheduledOperation const&) = default; + BasicScheduledOperation& + operator=(BasicScheduledOperation const&) = default; + BasicScheduledOperation(BasicScheduledOperation&&) = default; + BasicScheduledOperation& + operator=(BasicScheduledOperation&&) = default; + [[nodiscard]] auto get() { @@ -149,7 +161,8 @@ struct BasicScheduledOperation { * @tparam StopSourceType The type of the stop source */ template -class StoppableOperation : public impl::BasicOperation> { +class StoppableOperation : public impl::BasicOperation>, + public util::MoveTracker { using OutcomeType = StoppableOutcome; StopSourceType stopSource_; @@ -165,6 +178,19 @@ public: { } + ~StoppableOperation() + { + if (not wasMoved()) + requestStop(); + } + + StoppableOperation(StoppableOperation const&) = delete; + StoppableOperation& + operator=(StoppableOperation const&) = delete; + StoppableOperation(StoppableOperation&&) = default; + StoppableOperation& + operator=(StoppableOperation&&) = default; + /** @brief Requests the operation to stop */ void requestStop() noexcept @@ -199,7 +225,7 @@ using ScheduledOperation = impl::BasicScheduledOperation; * @tparam CtxType The type of the execution context */ template -class RepeatingOperation { +class RepeatingOperation : public util::MoveTracker { util::Repeat repeat_; public: @@ -217,6 +243,12 @@ public: repeat_.start(interval, std::forward(fn)); } + ~RepeatingOperation() + { + if (not wasMoved()) + abort(); + } + RepeatingOperation(RepeatingOperation const&) = delete; RepeatingOperation& operator=(RepeatingOperation const&) = delete; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index f1eda9b2b..c1371f594 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -114,8 +114,6 @@ target_sources( rpc/RPCHelpersTests.cpp rpc/WorkQueueTests.cpp test_data/SslCert.cpp - util/AccountUtilsTests.cpp - util/AssertTests.cpp # Async framework util/async/AnyExecutionContextTests.cpp util/async/AnyOperationTests.cpp @@ -140,6 +138,10 @@ target_sources( util/requests/RequestBuilderTests.cpp util/requests/SslContextTests.cpp util/requests/WsConnectionTests.cpp + # Common utils + util/AccountUtilsTests.cpp + util/AssertTests.cpp + util/MoveTrackerTests.cpp util/RandomTests.cpp util/RetryTests.cpp util/RepeatTests.cpp diff --git a/tests/unit/util/MoveTrackerTests.cpp b/tests/unit/util/MoveTrackerTests.cpp new file mode 100644 index 000000000..366f5a49a --- /dev/null +++ b/tests/unit/util/MoveTrackerTests.cpp @@ -0,0 +1,68 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "util/MoveTracker.hpp" + +#include + +#include + +namespace { +struct MoveMe : util::MoveTracker { + using MoveTracker::wasMoved; // expose as public for tests +}; +} // namespace + +TEST(MoveTrackerTests, SimpleChecks) +{ + auto moveMe = MoveMe(); + EXPECT_FALSE(moveMe.wasMoved()); + + auto other = std::move(moveMe); + EXPECT_TRUE(moveMe.wasMoved()); + EXPECT_FALSE(other.wasMoved()); +} + +TEST(MoveTrackerTests, SupportReuse) +{ + auto original = MoveMe(); + auto other = std::move(original); + + original = std::move(other); + EXPECT_FALSE(original.wasMoved()); + EXPECT_TRUE(other.wasMoved()); +} + +TEST(MoveTrackerTests, SelfMove) +{ + auto original = MoveMe(); + [&](MoveMe& from) { original = std::move(from); }(original); // avoids the compiler catching self-move + + EXPECT_FALSE(original.wasMoved()); +} + +TEST(MoveTrackerTests, SelfMoveAfterWasMoved) +{ + auto original = MoveMe(); + [[maybe_unused]] auto fake = std::move(original); + + [&](MoveMe& from) { original = std::move(from); }(original); // avoids the compiler catching self-move + + EXPECT_TRUE(original.wasMoved()); +} diff --git a/tests/unit/util/async/AsyncExecutionContextTests.cpp b/tests/unit/util/async/AsyncExecutionContextTests.cpp index 04ba5fb94..c78ce115f 100644 --- a/tests/unit/util/async/AsyncExecutionContextTests.cpp +++ b/tests/unit/util/async/AsyncExecutionContextTests.cpp @@ -34,8 +34,6 @@ using namespace util::async; using ::testing::Types; -using ExecutionContextTypes = Types; - template struct ExecutionContextTests : public ::testing::Test { using ExecutionContextType = T; @@ -48,7 +46,15 @@ struct ExecutionContextTests : public ::testing::Test { } }; +// Suite for tests to be ran against all context types but SyncExecutionContext +template +using AsyncExecutionContextTests = ExecutionContextTests; + +using ExecutionContextTypes = Types; +using AsyncExecutionContextTypes = Types; + TYPED_TEST_CASE(ExecutionContextTests, ExecutionContextTypes); +TYPED_TEST_CASE(AsyncExecutionContextTests, AsyncExecutionContextTypes); TYPED_TEST(ExecutionContextTests, move) { @@ -149,6 +155,26 @@ TYPED_TEST(ExecutionContextTests, timerCancel) EXPECT_EQ(value, 42); } +TYPED_TEST(ExecutionContextTests, timerAutoCancels) +{ + auto value = 0; + std::binary_semaphore sem{0}; + { + auto res = this->ctx.scheduleAfter( + std::chrono::milliseconds(1), + [&value, &sem]([[maybe_unused]] auto stopRequested, auto cancelled) { + if (cancelled) + value = 42; + + sem.release(); + } + ); + } // res goes out of scope and cancels the timer + + sem.acquire(); + EXPECT_EQ(value, 42); +} + TYPED_TEST(ExecutionContextTests, timerStdException) { auto res = @@ -247,6 +273,46 @@ TYPED_TEST(ExecutionContextTests, strandWithTimeout) EXPECT_EQ(res.get().value(), 42); } +TYPED_TEST(AsyncExecutionContextTests, executeAutoAborts) +{ + auto value = 0; + std::binary_semaphore sem{0}; + + { + auto res = this->ctx.execute([&](auto stopRequested) { + while (not stopRequested) + ; + value = 42; + sem.release(); + }); + } // res goes out of scope and aborts operation + + sem.acquire(); + EXPECT_EQ(value, 42); +} + +TYPED_TEST(AsyncExecutionContextTests, repeatingOperationAutoAborts) +{ + auto const repeatDelay = std::chrono::milliseconds{1}; + auto const timeout = std::chrono::milliseconds{15}; + auto callCount = 0uz; + auto timeSpentMs = 0u; + + { + auto res = this->ctx.executeRepeatedly(repeatDelay, [&] { ++callCount; }); + timeSpentMs = util::timed([timeout] { std::this_thread::sleep_for(timeout); }); // calculate actual time spent + } // res goes out of scope and automatically aborts the repeating operation + + // double the delay so that if abort did not happen we will fail below expectations + std::this_thread::sleep_for(timeout); + + auto const expectedPureCalls = timeout.count() / repeatDelay.count(); + auto const expectedActualCount = timeSpentMs / repeatDelay.count(); + + EXPECT_GE(callCount, expectedPureCalls / 2u); // expect at least half of the scheduled calls + EXPECT_LE(callCount, expectedActualCount); // never should be called more times than possible before timeout +} + using NoErrorHandlerSyncExecutionContext = BasicExecutionContext< impl::SameThreadContext, impl::BasicStopSource, From c0d52723c9befa07de790500f08b22dfdcec8b64 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 10 Jan 2025 09:06:05 +0000 Subject: [PATCH 04/31] style: clang-tidy auto fixes (#1817) Fixes #1816. Please review and commit clang-tidy fixes. Co-authored-by: kuznetsss <15742918+kuznetsss@users.noreply.github.com> --- src/etlng/impl/Loading.cpp | 8 -------- src/util/async/Operation.hpp | 6 +++--- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/etlng/impl/Loading.cpp b/src/etlng/impl/Loading.cpp index c35841e04..e8608dbed 100644 --- a/src/etlng/impl/Loading.cpp +++ b/src/etlng/impl/Loading.cpp @@ -30,15 +30,7 @@ #include "util/Profiler.hpp" #include "util/log/Logger.hpp" -#include -#include -#include -#include -#include #include -#include -#include -#include #include #include diff --git a/src/util/async/Operation.hpp b/src/util/async/Operation.hpp index 1ad0c41fe..8bb4a531d 100644 --- a/src/util/async/Operation.hpp +++ b/src/util/async/Operation.hpp @@ -104,7 +104,7 @@ struct BasicScheduledOperation : util::MoveTracker { { } - ~BasicScheduledOperation() + ~BasicScheduledOperation() override { if (not wasMoved()) abort(); @@ -178,7 +178,7 @@ public: { } - ~StoppableOperation() + ~StoppableOperation() override { if (not wasMoved()) requestStop(); @@ -243,7 +243,7 @@ public: repeat_.start(interval, std::forward(fn)); } - ~RepeatingOperation() + ~RepeatingOperation() override { if (not wasMoved()) abort(); From 91c00e781a278286dab7ee322974fd2cc0aadffe Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Fri, 10 Jan 2025 15:47:48 +0000 Subject: [PATCH 05/31] fix: Silence expected use after move warnings (#1819) Fixes #1818 --- tests/unit/util/MoveTrackerTests.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/util/MoveTrackerTests.cpp b/tests/unit/util/MoveTrackerTests.cpp index 366f5a49a..292206867 100644 --- a/tests/unit/util/MoveTrackerTests.cpp +++ b/tests/unit/util/MoveTrackerTests.cpp @@ -35,7 +35,7 @@ TEST(MoveTrackerTests, SimpleChecks) EXPECT_FALSE(moveMe.wasMoved()); auto other = std::move(moveMe); - EXPECT_TRUE(moveMe.wasMoved()); + EXPECT_TRUE(moveMe.wasMoved()); // NOLINT(bugprone-use-after-move) EXPECT_FALSE(other.wasMoved()); } @@ -46,7 +46,7 @@ TEST(MoveTrackerTests, SupportReuse) original = std::move(other); EXPECT_FALSE(original.wasMoved()); - EXPECT_TRUE(other.wasMoved()); + EXPECT_TRUE(other.wasMoved()); // NOLINT(bugprone-use-after-move) } TEST(MoveTrackerTests, SelfMove) @@ -62,7 +62,8 @@ TEST(MoveTrackerTests, SelfMoveAfterWasMoved) auto original = MoveMe(); [[maybe_unused]] auto fake = std::move(original); + // NOLINTNEXTLINE(bugprone-use-after-move) [&](MoveMe& from) { original = std::move(from); }(original); // avoids the compiler catching self-move - EXPECT_TRUE(original.wasMoved()); + EXPECT_TRUE(original.wasMoved()); // NOLINT(bugprone-use-after-move) } From f1698c55ff8073b44cc12a8ebcc6eb03a209aced Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Mon, 13 Jan 2025 09:57:11 -0500 Subject: [PATCH 06/31] feat: add config verify flag (#1814) fixes #1806 --- src/app/CliArgs.cpp | 4 ++ src/app/CliArgs.hpp | 9 ++- src/app/VerifyConfig.hpp | 55 ++++++++++++++++ src/main/Main.cpp | 35 +++++------ src/util/newconfig/ConfigDefinition.hpp | 1 + .../common/util/newconfig/FakeConfigData.hpp | 8 +++ tests/unit/CMakeLists.txt | 1 + tests/unit/app/CliArgsTests.cpp | 53 ++++++++++++++-- tests/unit/app/VerifyConfigTests.cpp | 62 +++++++++++++++++++ 9 files changed, 201 insertions(+), 27 deletions(-) create mode 100644 src/app/VerifyConfig.hpp create mode 100644 tests/unit/app/VerifyConfigTests.cpp diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 0c973dee6..4673c9f90 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -47,6 +47,7 @@ CliArgs::parse(int argc, char const* argv[]) ("conf,c", po::value()->default_value(kDEFAULT_CONFIG_PATH), "configuration file") ("ng-web-server,w", "Use ng-web-server") ("migrate", po::value(), "start migration helper") + ("verify", "Checks the validity of config values") ; // clang-format on po::positional_options_description positional; @@ -75,6 +76,9 @@ CliArgs::parse(int argc, char const* argv[]) return Action{Action::Migrate{.configPath = std::move(configPath), .subCmd = MigrateSubCmd::migration(opt)}}; } + if (parsed.count("verify") != 0u) + return Action{Action::VerifyConfig{.configPath = std::move(configPath)}}; + return Action{Action::Run{.configPath = std::move(configPath), .useNgWebServer = parsed.count("ng-web-server") != 0} }; } diff --git a/src/app/CliArgs.hpp b/src/app/CliArgs.hpp index 2142ad3f6..8f2a8c359 100644 --- a/src/app/CliArgs.hpp +++ b/src/app/CliArgs.hpp @@ -59,6 +59,11 @@ public: MigrateSubCmd subCmd; }; + /** @brief Verify Config action. */ + struct VerifyConfig { + std::string configPath; + }; + /** * @brief Construct an action from a Run. * @@ -66,7 +71,7 @@ public: */ template requires std::is_same_v or std::is_same_v or - std::is_same_v + std::is_same_v or std::is_same_v explicit Action(ActionType&& action) : action_(std::forward(action)) { } @@ -86,7 +91,7 @@ public: } private: - std::variant action_; + std::variant action_; }; /** diff --git a/src/app/VerifyConfig.hpp b/src/app/VerifyConfig.hpp new file mode 100644 index 000000000..63c806dc9 --- /dev/null +++ b/src/app/VerifyConfig.hpp @@ -0,0 +1,55 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "util/newconfig/ConfigDefinition.hpp" +#include "util/newconfig/ConfigFileJson.hpp" + +#include +#include +#include + +namespace app { + +/** + * @brief Verifies user's config values are correct + * + * @param configPath The path to config + * @return true if config values are all correct, false otherwise + */ +inline bool +verifyConfig(std::string_view configPath) +{ + using namespace util::config; + + auto const json = ConfigFileJson::makeConfigFileJson(configPath); + if (!json.has_value()) { + std::cerr << json.error().error << std::endl; + return false; + } + auto const errors = gClioConfig.parse(json.value()); + if (errors.has_value()) { + for (auto const& err : errors.value()) + std::cerr << err.error << std::endl; + return false; + } + return true; +} +} // namespace app diff --git a/src/main/Main.cpp b/src/main/Main.cpp index 6a817f0dc..bd060b12d 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -19,12 +19,12 @@ #include "app/CliArgs.hpp" #include "app/ClioApplication.hpp" +#include "app/VerifyConfig.hpp" #include "migration/MigrationApplication.hpp" #include "rpc/common/impl/HandlerProvider.hpp" #include "util/TerminationHandler.hpp" #include "util/log/Logger.hpp" #include "util/newconfig/ConfigDefinition.hpp" -#include "util/newconfig/ConfigFileJson.hpp" #include #include @@ -40,34 +40,27 @@ try { auto const action = app::CliArgs::parse(argc, argv); return action.apply( [](app::CliArgs::Action::Exit const& exit) { return exit.exitCode; }, + [](app::CliArgs::Action::VerifyConfig const& verify) { + if (app::verifyConfig(verify.configPath)) { + std::cout << "Config is correct" << "\n"; + return EXIT_SUCCESS; + } + return EXIT_FAILURE; + }, [](app::CliArgs::Action::Run const& run) { - auto const json = ConfigFileJson::makeConfigFileJson(run.configPath); - if (!json.has_value()) { - std::cerr << json.error().error << std::endl; + auto const res = app::verifyConfig(run.configPath); + if (res != EXIT_SUCCESS) return EXIT_FAILURE; - } - auto const errors = gClioConfig.parse(json.value()); - if (errors.has_value()) { - for (auto const& err : errors.value()) - std::cerr << err.error << std::endl; - return EXIT_FAILURE; - } + util::LogService::init(gClioConfig); app::ClioApplication clio{gClioConfig}; return clio.run(run.useNgWebServer); }, [](app::CliArgs::Action::Migrate const& migrate) { - auto const json = ConfigFileJson::makeConfigFileJson(migrate.configPath); - if (!json.has_value()) { - std::cerr << json.error().error << std::endl; + auto const res = app::verifyConfig(migrate.configPath); + if (res != EXIT_SUCCESS) return EXIT_FAILURE; - } - auto const errors = gClioConfig.parse(json.value()); - if (errors.has_value()) { - for (auto const& err : errors.value()) - std::cerr << err.error << std::endl; - return EXIT_FAILURE; - } + util::LogService::init(gClioConfig); app::MigratorApplication migrator{gClioConfig, migrate.subCmd}; return migrator.run(); diff --git a/src/util/newconfig/ConfigDefinition.hpp b/src/util/newconfig/ConfigDefinition.hpp index 2766e767e..69fb1b6eb 100644 --- a/src/util/newconfig/ConfigDefinition.hpp +++ b/src/util/newconfig/ConfigDefinition.hpp @@ -416,6 +416,7 @@ static ClioConfigDefinition gClioConfig = ClioConfigDefinition{ ConfigValue{ConfigType::Integer}.defaultValue(rpc::kAPI_VERSION_MIN).withConstraint(gValidateApiVersion)}, {"api_version.max", ConfigValue{ConfigType::Integer}.defaultValue(rpc::kAPI_VERSION_MAX).withConstraint(gValidateApiVersion)}, + {"migration.full_scan_threads", ConfigValue{ConfigType::Integer}.defaultValue(2).withConstraint(gValidateUint32)}, {"migration.full_scan_jobs", ConfigValue{ConfigType::Integer}.defaultValue(4).withConstraint(gValidateUint32)}, {"migration.cursors_per_job", ConfigValue{ConfigType::Integer}.defaultValue(100).withConstraint(gValidateUint32)}}, diff --git a/tests/common/util/newconfig/FakeConfigData.hpp b/tests/common/util/newconfig/FakeConfigData.hpp index 4167e2b2e..f7340276c 100644 --- a/tests/common/util/newconfig/FakeConfigData.hpp +++ b/tests/common/util/newconfig/FakeConfigData.hpp @@ -208,3 +208,11 @@ static constexpr auto kINVALID_JSON_DATA = R"JSON({ "withDefault" : "0.0" } })JSON"; + +// used to Verify Config test +static constexpr auto kVALID_JSON_DATA = R"JSON({ + "server": { + "ip": "0.0.0.0", + "port": 51233 + } +})JSON"; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index c1371f594..fe74c1b83 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -5,6 +5,7 @@ target_sources( PRIVATE # Common ConfigTests.cpp app/CliArgsTests.cpp + app/VerifyConfigTests.cpp app/WebHandlersTests.cpp data/AmendmentCenterTests.cpp data/BackendCountersTests.cpp diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index 437f20157..d6e098420 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -32,6 +32,7 @@ struct CliArgsTests : testing::Test { testing::StrictMock> onRunMock; testing::StrictMock> onExitMock; testing::StrictMock> onMigrateMock; + testing::StrictMock> onVerifyMock; }; TEST_F(CliArgsTests, Parse_NoArgs) @@ -46,7 +47,13 @@ TEST_F(CliArgsTests, Parse_NoArgs) return returnCode; }); EXPECT_EQ( - action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), returnCode + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), + returnCode ); } @@ -62,7 +69,12 @@ TEST_F(CliArgsTests, Parse_NgWebServer) return returnCode; }); EXPECT_EQ( - action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), returnCode ); } @@ -79,7 +91,12 @@ TEST_F(CliArgsTests, Parse_VersionHelp) EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); EXPECT_EQ( - action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), EXIT_SUCCESS ); } @@ -97,6 +114,34 @@ TEST_F(CliArgsTests, Parse_Config) return returnCode; }); EXPECT_EQ( - action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), returnCode + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), + returnCode + ); +} + +TEST_F(CliArgsTests, Parse_VerifyConfig) +{ + std::string_view configPath = "some_config_path"; + std::array argv{"clio_server", configPath.data(), "--verify"}; // NOLINT(bugprone-suspicious-stringview-data-usage) + auto const action = CliArgs::parse(argv.size(), argv.data()); + + int const returnCode = 123; + EXPECT_CALL(onVerifyMock, Call).WillOnce([&configPath](CliArgs::Action::VerifyConfig const& verify) { + EXPECT_EQ(verify.configPath, configPath); + return returnCode; + }); + EXPECT_EQ( + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), + returnCode ); } diff --git a/tests/unit/app/VerifyConfigTests.cpp b/tests/unit/app/VerifyConfigTests.cpp new file mode 100644 index 000000000..9dad96286 --- /dev/null +++ b/tests/unit/app/VerifyConfigTests.cpp @@ -0,0 +1,62 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "app/VerifyConfig.hpp" +#include "util/TmpFile.hpp" +#include "util/newconfig/FakeConfigData.hpp" + +#include + +using namespace app; +using namespace util::config; + +TEST(VerifyConfigTest, InvalidConfig) +{ + auto const tmpConfigFile = TmpFile(kJSON_DATA); + + // false because json data(kJSON_DATA) is not compatible with current configDefintion + EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); +} + +TEST(VerifyConfigTest, ValidConfig) +{ + auto const tmpConfigFile = TmpFile(kVALID_JSON_DATA); + + // current example config should always be compatible with configDefinition + EXPECT_TRUE(verifyConfig(tmpConfigFile.path)); +} + +TEST(VerifyConfigTest, ConfigFileNotExist) +{ + EXPECT_FALSE(verifyConfig("doesn't exist Config File")); +} + +TEST(VerifyConfigTest, InvalidJsonFile) +{ + // invalid json because extra "," after 51233 + static constexpr auto kINVALID_JSON = R"({ + "server": { + "ip": "0.0.0.0", + "port": 51233, + } + })"; + auto const tmpConfigFile = TmpFile(kINVALID_JSON); + + EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); +} From 9659d98140eee7281083a3a931743605cd00c19b Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 13 Jan 2025 16:29:45 +0000 Subject: [PATCH 07/31] fix: Reading log_channels levels from config (#1821) --- src/util/log/Logger.cpp | 6 +- tests/common/util/LoggerFixtures.hpp | 2 +- tests/unit/LoggerTests.cpp | 112 +++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/src/util/log/Logger.cpp b/src/util/log/Logger.cpp index dfd1086f0..3f9a35dda 100644 --- a/src/util/log/Logger.cpp +++ b/src/util/log/Logger.cpp @@ -167,12 +167,12 @@ LogService::init(config::ClioConfigDefinition const& config) auto const overrides = config.getArray("log_channels"); for (auto it = overrides.begin(); it != overrides.end(); ++it) { - auto const& cfg = *it; - auto name = cfg.get("channel"); + auto const& channelConfig = *it; + auto const name = channelConfig.get("channel"); if (std::count(std::begin(Logger::kCHANNELS), std::end(Logger::kCHANNELS), name) == 0) throw std::runtime_error("Can't override settings for log channel " + name + ": invalid channel"); - minSeverity[name] = getSeverityLevel(config.get("log_level")); + minSeverity[name] = getSeverityLevel(channelConfig.get("log_level")); } auto logFilter = [minSeverity = std::move(minSeverity), diff --git a/tests/common/util/LoggerFixtures.hpp b/tests/common/util/LoggerFixtures.hpp index e19a23930..93c65c04c 100644 --- a/tests/common/util/LoggerFixtures.hpp +++ b/tests/common/util/LoggerFixtures.hpp @@ -91,7 +91,7 @@ protected: checkEqual(std::string expected) { auto value = buffer_.getStrAndReset(); - ASSERT_EQ(value, expected + '\n'); + ASSERT_EQ(value, expected + '\n') << "Got: " << value; } void diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index 4c9718f79..2624e8b13 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -19,8 +19,20 @@ #include "util/LoggerFixtures.hpp" #include "util/log/Logger.hpp" +#include "util/newconfig/Array.hpp" +#include "util/newconfig/ConfigConstraints.hpp" +#include "util/newconfig/ConfigDefinition.hpp" +#include "util/newconfig/ConfigFileJson.hpp" +#include "util/newconfig/ConfigValue.hpp" +#include "util/newconfig/Types.hpp" +#include +#include +#include #include + +#include +#include using namespace util; // Used as a fixture for tests with enabled logging @@ -56,6 +68,106 @@ TEST_F(LoggerTest, Filtering) checkEqual("Trace:TRC Trace line logged for 'Trace' component"); } +using util::config::Array; +using util::config::ConfigFileJson; +using util::config::ConfigType; +using util::config::ConfigValue; + +struct LoggerInitTest : LoggerTest { +protected: + util::config::ClioConfigDefinition config_{ + {"log_channels.[].channel", Array{ConfigValue{ConfigType::String}.optional()}}, + {"log_channels.[].log_level", Array{ConfigValue{ConfigType::String}.optional()}}, + + {"log_level", ConfigValue{ConfigType::String}.defaultValue("info")}, + + {"log_format", + ConfigValue{ConfigType::String}.defaultValue( + R"(%TimeStamp% (%SourceLocation%) [%ThreadID%] %Channel%:%Severity% %Message%)" + )}, + + {"log_to_console", ConfigValue{ConfigType::Boolean}.defaultValue(false)}, + + {"log_directory", ConfigValue{ConfigType::String}.optional()}, + + {"log_rotation_size", ConfigValue{ConfigType::Integer}.defaultValue(2048)}, + + {"log_directory_max_size", ConfigValue{ConfigType::Integer}.defaultValue(50 * 1024)}, + + {"log_rotation_hour_interval", ConfigValue{ConfigType::Integer}.defaultValue(12)}, + + {"log_tag_style", ConfigValue{ConfigType::String}.defaultValue("none")}, + }; +}; + +TEST_F(LoggerInitTest, DefaultLogLevel) +{ + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{{"log_level", "warn"}}}); + ASSERT_FALSE(parsingErrors.has_value()); + std::string const logString = "some log"; + + LogService::init(config_); + for (auto const& channel : Logger::kCHANNELS) { + Logger const log{channel}; + log.trace() << logString; + checkEmpty(); + + log.debug() << logString; + checkEmpty(); + + log.info() << logString; + checkEmpty(); + + log.warn() << logString; + checkEqual(fmt::format("{}:WRN {}", channel, logString)); + + log.error() << logString; + checkEqual(fmt::format("{}:ERR {}", channel, logString)); + } +} + +TEST_F(LoggerInitTest, ChannelLogLevel) +{ + std::string const configStr = R"json( + { + "log_level": "error", + "log_channels": [ + { + "channel": "Backend", + "log_level": "warning" + } + ] + } + )json"; + + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::parse(configStr).as_object()}); + ASSERT_FALSE(parsingErrors.has_value()); + std::string const logString = "some log"; + + LogService::init(config_); + for (auto const& channel : Logger::kCHANNELS) { + Logger const log{channel}; + log.trace() << logString; + checkEmpty(); + + log.debug() << logString; + checkEmpty(); + + log.info() << logString; + checkEmpty(); + + log.warn() << logString; + if (std::string_view{channel} == "Backend") { + checkEqual(fmt::format("{}:WRN {}", channel, logString)); + } else { + checkEmpty(); + } + + log.error() << "some log"; + checkEqual(fmt::format("{}:ERR {}", channel, logString)); + } +} + #ifndef COVERAGE_ENABLED TEST_F(LoggerTest, LOGMacro) { From c47b96bc6850ef284ce72e427ed0d1caac7574f0 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Mon, 13 Jan 2025 12:58:20 -0500 Subject: [PATCH 08/31] fix: comment from verify config PR (#1823) PR here: https://github.com/XRPLF/clio/pull/1814# --- src/app/VerifyConfig.hpp | 6 ++++-- src/main/Main.cpp | 2 +- tests/common/util/newconfig/FakeConfigData.hpp | 8 -------- tests/unit/app/VerifyConfigTests.cpp | 7 +++++++ 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/app/VerifyConfig.hpp b/src/app/VerifyConfig.hpp index 63c806dc9..6a540ed7b 100644 --- a/src/app/VerifyConfig.hpp +++ b/src/app/VerifyConfig.hpp @@ -41,13 +41,15 @@ verifyConfig(std::string_view configPath) auto const json = ConfigFileJson::makeConfigFileJson(configPath); if (!json.has_value()) { - std::cerr << json.error().error << std::endl; + std::cerr << "Error parsing json from config: " << configPath << "\n" << json.error().error << std::endl; return false; } auto const errors = gClioConfig.parse(json.value()); if (errors.has_value()) { - for (auto const& err : errors.value()) + for (auto const& err : errors.value()) { + std::cerr << "Issues found in provided config '" << configPath << "':\n"; std::cerr << err.error << std::endl; + } return false; } return true; diff --git a/src/main/Main.cpp b/src/main/Main.cpp index bd060b12d..b2ce539a6 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -42,7 +42,7 @@ try { [](app::CliArgs::Action::Exit const& exit) { return exit.exitCode; }, [](app::CliArgs::Action::VerifyConfig const& verify) { if (app::verifyConfig(verify.configPath)) { - std::cout << "Config is correct" << "\n"; + std::cout << "Config " << verify.configPath << " is correct" << "\n"; return EXIT_SUCCESS; } return EXIT_FAILURE; diff --git a/tests/common/util/newconfig/FakeConfigData.hpp b/tests/common/util/newconfig/FakeConfigData.hpp index f7340276c..4167e2b2e 100644 --- a/tests/common/util/newconfig/FakeConfigData.hpp +++ b/tests/common/util/newconfig/FakeConfigData.hpp @@ -208,11 +208,3 @@ static constexpr auto kINVALID_JSON_DATA = R"JSON({ "withDefault" : "0.0" } })JSON"; - -// used to Verify Config test -static constexpr auto kVALID_JSON_DATA = R"JSON({ - "server": { - "ip": "0.0.0.0", - "port": 51233 - } -})JSON"; diff --git a/tests/unit/app/VerifyConfigTests.cpp b/tests/unit/app/VerifyConfigTests.cpp index 9dad96286..b9d00c94f 100644 --- a/tests/unit/app/VerifyConfigTests.cpp +++ b/tests/unit/app/VerifyConfigTests.cpp @@ -36,6 +36,13 @@ TEST(VerifyConfigTest, InvalidConfig) TEST(VerifyConfigTest, ValidConfig) { + // used to Verify Config test + static constexpr auto kVALID_JSON_DATA = R"JSON({ + "server": { + "ip": "0.0.0.0", + "port": 51233 + } + })JSON"; auto const tmpConfigFile = TmpFile(kVALID_JSON_DATA); // current example config should always be compatible with configDefinition From 2cf849dd12231bc1ea370617656594b9e5a91b82 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 14 Jan 2025 15:50:59 +0000 Subject: [PATCH 09/31] feat: ETLng scheduling (#1820) For #1596 --- src/etlng/Models.hpp | 16 +++ src/etlng/SchedulerInterface.hpp | 42 ++++++ src/etlng/impl/Scheduling.hpp | 151 +++++++++++++++++++++ tests/unit/CMakeLists.txt | 1 + tests/unit/etlng/SchedulingTests.cpp | 187 +++++++++++++++++++++++++++ 5 files changed, 397 insertions(+) create mode 100644 src/etlng/SchedulerInterface.hpp create mode 100644 src/etlng/impl/Scheduling.hpp create mode 100644 tests/unit/etlng/SchedulingTests.cpp diff --git a/src/etlng/Models.hpp b/src/etlng/Models.hpp index afe6238b0..253ba8030 100644 --- a/src/etlng/Models.hpp +++ b/src/etlng/Models.hpp @@ -174,4 +174,20 @@ struct LedgerData { } }; +/** + * @brief Represents a task for the extractors + */ +struct Task { + /** + * @brief Represents the priority of the task + */ + enum class Priority : uint8_t { + Lower = 0u, + Higher = 1u, + }; + + Priority priority; + uint32_t seq; +}; + } // namespace etlng::model diff --git a/src/etlng/SchedulerInterface.hpp b/src/etlng/SchedulerInterface.hpp new file mode 100644 index 000000000..606757b60 --- /dev/null +++ b/src/etlng/SchedulerInterface.hpp @@ -0,0 +1,42 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "etlng/Models.hpp" + +#include + +namespace etlng { + +/** + * @brief The interface of a scheduler for the extraction proccess + */ +struct SchedulerInterface { + virtual ~SchedulerInterface() = default; + + /** + * @brief Attempt to obtain the next task + * @return A task if one exists; std::nullopt otherwise + */ + [[nodiscard]] virtual std::optional + next() = 0; +}; + +} // namespace etlng diff --git a/src/etlng/impl/Scheduling.hpp b/src/etlng/impl/Scheduling.hpp new file mode 100644 index 000000000..7e16aa9c0 --- /dev/null +++ b/src/etlng/impl/Scheduling.hpp @@ -0,0 +1,151 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "etl/NetworkValidatedLedgersInterface.hpp" +#include "etlng/Models.hpp" +#include "etlng/SchedulerInterface.hpp" + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace etlng::impl { + +template +concept SomeScheduler = std::is_base_of_v>; + +class ForwardScheduler : public SchedulerInterface { + std::reference_wrapper ledgers_; + + uint32_t startSeq_; + std::optional maxSeq_; + std::atomic_uint32_t seq_; + +public: + ForwardScheduler(ForwardScheduler const& other) + : ledgers_(other.ledgers_), startSeq_(other.startSeq_), maxSeq_(other.maxSeq_), seq_(other.seq_.load()) + { + } + + ForwardScheduler( + std::reference_wrapper ledgers, + uint32_t startSeq, + std::optional maxSeq = std::nullopt + ) + : ledgers_(ledgers), startSeq_(startSeq), maxSeq_(maxSeq), seq_(startSeq) + { + } + + [[nodiscard]] std::optional + next() override + { + static constexpr auto kMAX = std::numeric_limits::max(); + uint32_t currentSeq = seq_; + + if (ledgers_.get().getMostRecent() >= currentSeq) { + while (currentSeq < maxSeq_.value_or(kMAX)) { + if (seq_.compare_exchange_weak(currentSeq, currentSeq + 1u, std::memory_order_acq_rel)) { + return {{.priority = model::Task::Priority::Higher, .seq = currentSeq}}; + } + } + } + + return std::nullopt; + } +}; + +class BackfillScheduler : public SchedulerInterface { + uint32_t startSeq_; + uint32_t minSeq_ = 0u; + + std::atomic_uint32_t seq_; + +public: + BackfillScheduler(BackfillScheduler const& other) + : startSeq_(other.startSeq_), minSeq_(other.minSeq_), seq_(other.seq_.load()) + { + } + + BackfillScheduler(uint32_t startSeq, std::optional minSeq = std::nullopt) + : startSeq_(startSeq), minSeq_(minSeq.value_or(0)), seq_(startSeq) + { + } + + [[nodiscard]] std::optional + next() override + { + uint32_t currentSeq = seq_; + while (currentSeq > minSeq_) { + if (seq_.compare_exchange_weak(currentSeq, currentSeq - 1u, std::memory_order_acq_rel)) { + return {{.priority = model::Task::Priority::Lower, .seq = currentSeq}}; + } + } + + return std::nullopt; + } +}; + +template +class SchedulerChain : public SchedulerInterface { + std::tuple schedulers_; + +public: + template + requires(std::is_same_v and ...) + SchedulerChain(Ts&&... schedulers) : schedulers_(std::forward(schedulers)...) + { + } + + [[nodiscard]] std::optional + next() override + { + std::optional task; + auto const expand = [&](auto& s) { + if (task.has_value()) + return false; + + task = s.next(); + return task.has_value(); + }; + + std::apply([&expand](auto&&... xs) { (... || expand(xs)); }, schedulers_); + + return task; + } +}; + +static auto +makeScheduler(SomeScheduler auto&&... schedulers) +{ + return std::make_unique...>>( + std::forward(schedulers)... + ); +} + +} // namespace etlng::impl diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index fe74c1b83..e9df2ad74 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -38,6 +38,7 @@ target_sources( etlng/ExtractionTests.cpp etlng/GrpcSourceTests.cpp etlng/RegistryTests.cpp + etlng/SchedulingTests.cpp etlng/LoadingTests.cpp # Feed util/BytesConverterTests.cpp diff --git a/tests/unit/etlng/SchedulingTests.cpp b/tests/unit/etlng/SchedulingTests.cpp new file mode 100644 index 000000000..46928fb05 --- /dev/null +++ b/tests/unit/etlng/SchedulingTests.cpp @@ -0,0 +1,187 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "etlng/Models.hpp" +#include "etlng/SchedulerInterface.hpp" +#include "etlng/impl/Loading.hpp" +#include "etlng/impl/Scheduling.hpp" +#include "util/LoggerFixtures.hpp" +#include "util/MockNetworkValidatedLedgers.hpp" + +#include +#include + +#include +#include +#include +#include + +using namespace etlng; +using namespace etlng::model; + +namespace { +class FakeScheduler : SchedulerInterface { + std::function()> generator_; + +public: + FakeScheduler(std::function()> generator) : generator_{std::move(generator)} + { + } + + [[nodiscard]] std::optional + next() override + { + return generator_(); + } +}; +} // namespace + +struct ForwardSchedulerTests : NoLoggerFixture { +protected: + MockNetworkValidatedLedgersPtr networkValidatedLedgers_; +}; + +TEST_F(ForwardSchedulerTests, ExhaustsSchedulerIfMostRecentLedgerIsNewerThanRequestedSequence) +{ + auto scheduler = impl::ForwardScheduler(*networkValidatedLedgers_, 0u, 10u); + EXPECT_CALL(*networkValidatedLedgers_, getMostRecent()).Times(11).WillRepeatedly(testing::Return(11u)); + + for (auto i = 0u; i < 10u; ++i) { + auto maybeTask = scheduler.next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + auto const empty = scheduler.next(); + EXPECT_FALSE(empty.has_value()); +} + +TEST_F(ForwardSchedulerTests, ReturnsNulloptIfMostRecentLedgerIsOlderThanRequestedSequence) +{ + auto scheduler = impl::ForwardScheduler(*networkValidatedLedgers_, 0u, 10u); + EXPECT_CALL(*networkValidatedLedgers_, getMostRecent()).Times(10).WillRepeatedly(testing::Return(4u)); + + for (auto i = 0u; i < 5u; ++i) { + auto const maybeTask = scheduler.next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + for (auto i = 0u; i < 5u; ++i) + EXPECT_FALSE(scheduler.next().has_value()); +} + +TEST(BackfillSchedulerTests, ExhaustsSchedulerUntilMinSeqReached) +{ + auto scheduler = impl::BackfillScheduler(10u, 5u); + + for (auto i = 10u; i > 5u; --i) { + auto maybeTask = scheduler.next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + auto const empty = scheduler.next(); + EXPECT_FALSE(empty.has_value()); +} + +TEST(BackfillSchedulerTests, ExhaustsSchedulerUntilDefaultMinValueReached) +{ + auto scheduler = impl::BackfillScheduler(10u); + + for (auto i = 10u; i > 0u; --i) { + auto const maybeTask = scheduler.next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + auto const empty = scheduler.next(); + EXPECT_FALSE(empty.has_value()); +} + +TEST(SchedulerChainTests, ExhaustsOneGenerator) +{ + auto generate = [stop = 10u, seq = 0u]() mutable { + std::optional task = std::nullopt; + if (seq < stop) + task = Task{.priority = model::Task::Priority::Lower, .seq = seq++}; + + return task; + }; + testing::MockFunction()> upToTenGen; + EXPECT_CALL(upToTenGen, Call()).Times(11).WillRepeatedly(testing::ByRef(generate)); + + auto scheduler = impl::makeScheduler(FakeScheduler(upToTenGen.AsStdFunction())); + + for (auto i = 0u; i < 10u; ++i) { + auto const maybeTask = scheduler->next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + auto const empty = scheduler->next(); + EXPECT_FALSE(empty.has_value()); +} + +TEST(SchedulerChainTests, ExhaustsFirstSchedulerBeforeUsingSecond) +{ + auto generateFirst = [stop = 10u, seq = 0u]() mutable { + std::optional task = std::nullopt; + if (seq < stop) + task = Task{.priority = model::Task::Priority::Lower, .seq = seq++}; + + return task; + }; + testing::MockFunction()> upToTenGen; + EXPECT_CALL(upToTenGen, Call()).Times(21).WillRepeatedly(testing::ByRef(generateFirst)); + + auto generateSecond = [seq = 10u]() mutable { + std::optional task = std::nullopt; + if (seq > 0u) + task = Task{.priority = model::Task::Priority::Lower, .seq = seq--}; + + return task; + }; + testing::MockFunction()> downToZeroGen; + EXPECT_CALL(downToZeroGen, Call()).Times(11).WillRepeatedly(testing::ByRef(generateSecond)); + + auto scheduler = + impl::makeScheduler(FakeScheduler(upToTenGen.AsStdFunction()), FakeScheduler(downToZeroGen.AsStdFunction())); + + for (auto i = 0u; i < 10u; ++i) { + auto const maybeTask = scheduler->next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + for (auto i = 10u; i > 0u; --i) { + auto const maybeTask = scheduler->next(); + + EXPECT_TRUE(maybeTask.has_value()); + EXPECT_EQ(maybeTask->seq, i); + } + + auto const empty = scheduler->next(); + EXPECT_FALSE(empty.has_value()); +} From 7834b63b5541f198334d8bd4e9278fd713c6e686 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Tue, 14 Jan 2025 16:55:05 +0000 Subject: [PATCH 10/31] fix: Check result of parsing config (#1829) --- src/app/VerifyConfig.hpp | 3 ++- src/main/Main.cpp | 8 +++----- tests/unit/app/VerifyConfigTests.cpp | 8 ++++---- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/app/VerifyConfig.hpp b/src/app/VerifyConfig.hpp index 6a540ed7b..9182072ec 100644 --- a/src/app/VerifyConfig.hpp +++ b/src/app/VerifyConfig.hpp @@ -35,7 +35,7 @@ namespace app { * @return true if config values are all correct, false otherwise */ inline bool -verifyConfig(std::string_view configPath) +parseConfig(std::string_view configPath) { using namespace util::config; @@ -54,4 +54,5 @@ verifyConfig(std::string_view configPath) } return true; } + } // namespace app diff --git a/src/main/Main.cpp b/src/main/Main.cpp index b2ce539a6..11c034e58 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -41,15 +41,14 @@ try { return action.apply( [](app::CliArgs::Action::Exit const& exit) { return exit.exitCode; }, [](app::CliArgs::Action::VerifyConfig const& verify) { - if (app::verifyConfig(verify.configPath)) { + if (app::parseConfig(verify.configPath)) { std::cout << "Config " << verify.configPath << " is correct" << "\n"; return EXIT_SUCCESS; } return EXIT_FAILURE; }, [](app::CliArgs::Action::Run const& run) { - auto const res = app::verifyConfig(run.configPath); - if (res != EXIT_SUCCESS) + if (not app::parseConfig(run.configPath)) return EXIT_FAILURE; util::LogService::init(gClioConfig); @@ -57,8 +56,7 @@ try { return clio.run(run.useNgWebServer); }, [](app::CliArgs::Action::Migrate const& migrate) { - auto const res = app::verifyConfig(migrate.configPath); - if (res != EXIT_SUCCESS) + if (not app::parseConfig(migrate.configPath)) return EXIT_FAILURE; util::LogService::init(gClioConfig); diff --git a/tests/unit/app/VerifyConfigTests.cpp b/tests/unit/app/VerifyConfigTests.cpp index b9d00c94f..929c5ac83 100644 --- a/tests/unit/app/VerifyConfigTests.cpp +++ b/tests/unit/app/VerifyConfigTests.cpp @@ -31,7 +31,7 @@ TEST(VerifyConfigTest, InvalidConfig) auto const tmpConfigFile = TmpFile(kJSON_DATA); // false because json data(kJSON_DATA) is not compatible with current configDefintion - EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); + EXPECT_FALSE(parseConfig(tmpConfigFile.path)); } TEST(VerifyConfigTest, ValidConfig) @@ -46,12 +46,12 @@ TEST(VerifyConfigTest, ValidConfig) auto const tmpConfigFile = TmpFile(kVALID_JSON_DATA); // current example config should always be compatible with configDefinition - EXPECT_TRUE(verifyConfig(tmpConfigFile.path)); + EXPECT_TRUE(parseConfig(tmpConfigFile.path)); } TEST(VerifyConfigTest, ConfigFileNotExist) { - EXPECT_FALSE(verifyConfig("doesn't exist Config File")); + EXPECT_FALSE(parseConfig("doesn't exist Config File")); } TEST(VerifyConfigTest, InvalidJsonFile) @@ -65,5 +65,5 @@ TEST(VerifyConfigTest, InvalidJsonFile) })"; auto const tmpConfigFile = TmpFile(kINVALID_JSON); - EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); + EXPECT_FALSE(parseConfig(tmpConfigFile.path)); } From 3e38ea9b48c6dc4a3e4c6ed212e504e7b85175b2 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Wed, 15 Jan 2025 10:56:50 -0500 Subject: [PATCH 11/31] fix: Add more constraints to config (#1831) Log file size and rotation should also not allowed to be 0. --- src/util/newconfig/ConfigConstraints.hpp | 4 ++++ src/util/newconfig/ConfigDefinition.hpp | 14 ++++++----- tests/unit/LoggerTests.cpp | 30 +++++++++++++++++++++--- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/util/newconfig/ConfigConstraints.hpp b/src/util/newconfig/ConfigConstraints.hpp index 85d7b48ea..315ea05f7 100644 --- a/src/util/newconfig/ConfigConstraints.hpp +++ b/src/util/newconfig/ConfigConstraints.hpp @@ -362,6 +362,10 @@ static constinit NumberValueConstraint gValidateUint16{ std::numeric_limits::min(), std::numeric_limits::max() }; + +// log file size minimum is 1mb, log rotation time minimum is 1hr +static constinit NumberValueConstraint gValidateLogSize{1, std::numeric_limits::max()}; +static constinit NumberValueConstraint gValidateLogRotationTime{1, std::numeric_limits::max()}; static constinit NumberValueConstraint gValidateUint32{ std::numeric_limits::min(), std::numeric_limits::max() diff --git a/src/util/newconfig/ConfigDefinition.hpp b/src/util/newconfig/ConfigDefinition.hpp index 69fb1b6eb..94a70b671 100644 --- a/src/util/newconfig/ConfigDefinition.hpp +++ b/src/util/newconfig/ConfigDefinition.hpp @@ -350,8 +350,9 @@ static ClioConfigDefinition gClioConfig = ClioConfigDefinition{ {"server.admin_password", ConfigValue{ConfigType::String}.optional()}, {"server.processing_policy", ConfigValue{ConfigType::String}.defaultValue("parallel").withConstraint(gValidateProcessingPolicy)}, - {"server.parallel_requests_limit", ConfigValue{ConfigType::Integer}.optional()}, - {"server.ws_max_sending_queue_size", ConfigValue{ConfigType::Integer}.defaultValue(1500)}, + {"server.parallel_requests_limit", ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateUint16)}, + {"server.ws_max_sending_queue_size", + ConfigValue{ConfigType::Integer}.defaultValue(1500).withConstraint(gValidateUint32)}, {"server.__ng_web_server", ConfigValue{ConfigType::Boolean}.defaultValue(false)}, {"prometheus.enabled", ConfigValue{ConfigType::Boolean}.defaultValue(true)}, @@ -387,12 +388,13 @@ static ClioConfigDefinition gClioConfig = ClioConfigDefinition{ {"log_directory", ConfigValue{ConfigType::String}.optional()}, - {"log_rotation_size", ConfigValue{ConfigType::Integer}.defaultValue(2048).withConstraint(gValidateUint32)}, + {"log_rotation_size", ConfigValue{ConfigType::Integer}.defaultValue(2048).withConstraint(gValidateLogSize)}, - {"log_directory_max_size", ConfigValue{ConfigType::Integer}.defaultValue(50 * 1024).withConstraint(gValidateUint32) - }, + {"log_directory_max_size", + ConfigValue{ConfigType::Integer}.defaultValue(50 * 1024).withConstraint(gValidateLogSize)}, - {"log_rotation_hour_interval", ConfigValue{ConfigType::Integer}.defaultValue(12).withConstraint(gValidateUint32)}, + {"log_rotation_hour_interval", + ConfigValue{ConfigType::Integer}.defaultValue(12).withConstraint(gValidateLogRotationTime)}, {"log_tag_style", ConfigValue{ConfigType::String}.defaultValue("none").withConstraint(gValidateLogTag)}, diff --git a/tests/unit/LoggerTests.cpp b/tests/unit/LoggerTests.cpp index 2624e8b13..a8f986bf5 100644 --- a/tests/unit/LoggerTests.cpp +++ b/tests/unit/LoggerTests.cpp @@ -31,8 +31,12 @@ #include #include +#include +#include +#include #include #include +#include using namespace util; // Used as a fixture for tests with enabled logging @@ -90,11 +94,14 @@ protected: {"log_directory", ConfigValue{ConfigType::String}.optional()}, - {"log_rotation_size", ConfigValue{ConfigType::Integer}.defaultValue(2048)}, + {"log_rotation_size", + ConfigValue{ConfigType::Integer}.defaultValue(2048).withConstraint(config::gValidateLogSize)}, - {"log_directory_max_size", ConfigValue{ConfigType::Integer}.defaultValue(50 * 1024)}, + {"log_directory_max_size", + ConfigValue{ConfigType::Integer}.defaultValue(50 * 1024).withConstraint(config::gValidateLogSize)}, - {"log_rotation_hour_interval", ConfigValue{ConfigType::Integer}.defaultValue(12)}, + {"log_rotation_hour_interval", + ConfigValue{ConfigType::Integer}.defaultValue(12).withConstraint(config::gValidateLogRotationTime)}, {"log_tag_style", ConfigValue{ConfigType::String}.defaultValue("none")}, }; @@ -168,6 +175,23 @@ TEST_F(LoggerInitTest, ChannelLogLevel) } } +TEST_F(LoggerInitTest, LogSizeAndHourRotationCannotBeZero) +{ + std::vector const keys{ + "log_rotation_hour_interval", "log_directory_max_size", "log_rotation_size" + }; + + auto const parsingErrors = + config_.parse(ConfigFileJson{boost::json::object{{keys[0], 0}, {keys[1], 0}, {keys[2], 0}}}); + ASSERT_TRUE(parsingErrors->size() == 3); + for (std::size_t i = 0; i < parsingErrors->size(); ++i) { + EXPECT_EQ( + (*parsingErrors)[i].error, + fmt::format("{} Number must be between 1 and {}", keys[i], std::numeric_limits::max()) + ); + } +} + #ifndef COVERAGE_ENABLED TEST_F(LoggerTest, LOGMacro) { From f64d8ecb7717b50f362d8a98c0606baf55fb967f Mon Sep 17 00:00:00 2001 From: cyan317 <120398799+cindyyan317@users.noreply.github.com> Date: Thu, 16 Jan 2025 14:36:03 +0000 Subject: [PATCH 12/31] fix: Copyright format (#1835) Aligned the copyright notice to match the format used in other files. Updated Copyright (c) 2022-2024 to Copyright (c) 2024 to ensure consistency. --- src/main/Main.cpp | 5 +++-- src/migration/MigrationApplication.cpp | 2 +- src/migration/MigrationApplication.hpp | 2 +- src/migration/MigrationManagerInterface.hpp | 2 +- src/migration/MigratiorStatus.hpp | 2 +- src/migration/MigratorStatus.cpp | 2 +- src/migration/cassandra/CassandraMigrationBackend.hpp | 2 +- src/migration/cassandra/CassandraMigrationManager.hpp | 2 +- src/migration/cassandra/impl/CassandraMigrationSchema.hpp | 2 +- src/migration/cassandra/impl/FullTableScanner.hpp | 2 +- src/migration/cassandra/impl/FullTableScannerAdapterBase.hpp | 2 +- src/migration/cassandra/impl/ObjectsAdapter.cpp | 2 +- src/migration/cassandra/impl/ObjectsAdapter.hpp | 2 +- src/migration/cassandra/impl/Spec.hpp | 2 +- src/migration/cassandra/impl/TransactionsAdapter.cpp | 2 +- src/migration/cassandra/impl/TransactionsAdapter.hpp | 2 +- src/migration/cassandra/impl/Types.hpp | 2 +- src/migration/impl/MigrationManagerBase.hpp | 2 +- src/migration/impl/MigrationManagerFactory.cpp | 2 +- src/migration/impl/MigrationManagerFactory.hpp | 2 +- src/migration/impl/MigratorsRegister.hpp | 2 +- src/migration/impl/Spec.hpp | 2 +- .../migration/cassandra/CassandraMigrationTestBackend.hpp | 2 +- tests/integration/migration/cassandra/DBRawData.cpp | 2 +- tests/integration/migration/cassandra/DBRawData.hpp | 2 +- .../migration/cassandra/ExampleDropTableMigrator.cpp | 2 +- .../migration/cassandra/ExampleDropTableMigrator.hpp | 2 +- .../migration/cassandra/ExampleLedgerMigrator.cpp | 2 +- .../migration/cassandra/ExampleLedgerMigrator.hpp | 2 +- .../migration/cassandra/ExampleObjectsMigrator.cpp | 2 +- .../migration/cassandra/ExampleObjectsMigrator.hpp | 2 +- .../migration/cassandra/ExampleTransactionsMigrator.cpp | 2 +- .../migration/cassandra/ExampleTransactionsMigrator.hpp | 2 +- tests/integration/util/CassandraDBHelper.cpp | 2 +- tests/integration/util/CassandraDBHelper.hpp | 2 +- 35 files changed, 37 insertions(+), 36 deletions(-) diff --git a/src/main/Main.cpp b/src/main/Main.cpp index 11c034e58..4dd39e712 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above @@ -42,7 +42,8 @@ try { [](app::CliArgs::Action::Exit const& exit) { return exit.exitCode; }, [](app::CliArgs::Action::VerifyConfig const& verify) { if (app::parseConfig(verify.configPath)) { - std::cout << "Config " << verify.configPath << " is correct" << "\n"; + std::cout << "Config " << verify.configPath << " is correct" + << "\n"; return EXIT_SUCCESS; } return EXIT_FAILURE; diff --git a/src/migration/MigrationApplication.cpp b/src/migration/MigrationApplication.cpp index 5b05a848c..c628316d9 100644 --- a/src/migration/MigrationApplication.cpp +++ b/src/migration/MigrationApplication.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/MigrationApplication.hpp b/src/migration/MigrationApplication.hpp index 81d9cc077..a38687dca 100644 --- a/src/migration/MigrationApplication.hpp +++ b/src/migration/MigrationApplication.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/MigrationManagerInterface.hpp b/src/migration/MigrationManagerInterface.hpp index 66eafef92..863c336ab 100644 --- a/src/migration/MigrationManagerInterface.hpp +++ b/src/migration/MigrationManagerInterface.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/MigratiorStatus.hpp b/src/migration/MigratiorStatus.hpp index c01b0fabc..747f7f0a8 100644 --- a/src/migration/MigratiorStatus.hpp +++ b/src/migration/MigratiorStatus.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/MigratorStatus.cpp b/src/migration/MigratorStatus.cpp index 75c66df95..cfc630377 100644 --- a/src/migration/MigratorStatus.cpp +++ b/src/migration/MigratorStatus.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/CassandraMigrationBackend.hpp b/src/migration/cassandra/CassandraMigrationBackend.hpp index f967d2f35..bbc39255a 100644 --- a/src/migration/cassandra/CassandraMigrationBackend.hpp +++ b/src/migration/cassandra/CassandraMigrationBackend.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/CassandraMigrationManager.hpp b/src/migration/cassandra/CassandraMigrationManager.hpp index e3a17a238..65b0f2212 100644 --- a/src/migration/cassandra/CassandraMigrationManager.hpp +++ b/src/migration/cassandra/CassandraMigrationManager.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/CassandraMigrationSchema.hpp b/src/migration/cassandra/impl/CassandraMigrationSchema.hpp index d998a6638..56fde4985 100644 --- a/src/migration/cassandra/impl/CassandraMigrationSchema.hpp +++ b/src/migration/cassandra/impl/CassandraMigrationSchema.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/FullTableScanner.hpp b/src/migration/cassandra/impl/FullTableScanner.hpp index 34261893f..8407760c0 100644 --- a/src/migration/cassandra/impl/FullTableScanner.hpp +++ b/src/migration/cassandra/impl/FullTableScanner.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/FullTableScannerAdapterBase.hpp b/src/migration/cassandra/impl/FullTableScannerAdapterBase.hpp index c0dc291e7..5373af47e 100644 --- a/src/migration/cassandra/impl/FullTableScannerAdapterBase.hpp +++ b/src/migration/cassandra/impl/FullTableScannerAdapterBase.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/ObjectsAdapter.cpp b/src/migration/cassandra/impl/ObjectsAdapter.cpp index 0a74a31d7..d7d60abbb 100644 --- a/src/migration/cassandra/impl/ObjectsAdapter.cpp +++ b/src/migration/cassandra/impl/ObjectsAdapter.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/ObjectsAdapter.hpp b/src/migration/cassandra/impl/ObjectsAdapter.hpp index 91463e3be..6d4362588 100644 --- a/src/migration/cassandra/impl/ObjectsAdapter.hpp +++ b/src/migration/cassandra/impl/ObjectsAdapter.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/Spec.hpp b/src/migration/cassandra/impl/Spec.hpp index c71956630..a7ed3748b 100644 --- a/src/migration/cassandra/impl/Spec.hpp +++ b/src/migration/cassandra/impl/Spec.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/TransactionsAdapter.cpp b/src/migration/cassandra/impl/TransactionsAdapter.cpp index c7b90bbfa..9aa9915b7 100644 --- a/src/migration/cassandra/impl/TransactionsAdapter.cpp +++ b/src/migration/cassandra/impl/TransactionsAdapter.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/TransactionsAdapter.hpp b/src/migration/cassandra/impl/TransactionsAdapter.hpp index 6bb27ee8a..ed1b92b2d 100644 --- a/src/migration/cassandra/impl/TransactionsAdapter.hpp +++ b/src/migration/cassandra/impl/TransactionsAdapter.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/cassandra/impl/Types.hpp b/src/migration/cassandra/impl/Types.hpp index 2d65956c4..df1755a0a 100644 --- a/src/migration/cassandra/impl/Types.hpp +++ b/src/migration/cassandra/impl/Types.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/impl/MigrationManagerBase.hpp b/src/migration/impl/MigrationManagerBase.hpp index c04c035ef..c4f2d61fe 100644 --- a/src/migration/impl/MigrationManagerBase.hpp +++ b/src/migration/impl/MigrationManagerBase.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/impl/MigrationManagerFactory.cpp b/src/migration/impl/MigrationManagerFactory.cpp index af4c550f9..9ab5d2343 100644 --- a/src/migration/impl/MigrationManagerFactory.cpp +++ b/src/migration/impl/MigrationManagerFactory.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/impl/MigrationManagerFactory.hpp b/src/migration/impl/MigrationManagerFactory.hpp index 936ac59b6..0cca4997d 100644 --- a/src/migration/impl/MigrationManagerFactory.hpp +++ b/src/migration/impl/MigrationManagerFactory.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/impl/MigratorsRegister.hpp b/src/migration/impl/MigratorsRegister.hpp index e01c6e0e6..b27479b53 100644 --- a/src/migration/impl/MigratorsRegister.hpp +++ b/src/migration/impl/MigratorsRegister.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/src/migration/impl/Spec.hpp b/src/migration/impl/Spec.hpp index 5d6c15f72..d9d379441 100644 --- a/src/migration/impl/Spec.hpp +++ b/src/migration/impl/Spec.hpp @@ -2,7 +2,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/CassandraMigrationTestBackend.hpp b/tests/integration/migration/cassandra/CassandraMigrationTestBackend.hpp index e10d0faa0..7fe9d00c9 100644 --- a/tests/integration/migration/cassandra/CassandraMigrationTestBackend.hpp +++ b/tests/integration/migration/cassandra/CassandraMigrationTestBackend.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/DBRawData.cpp b/tests/integration/migration/cassandra/DBRawData.cpp index 98fddc966..f6e581905 100644 --- a/tests/integration/migration/cassandra/DBRawData.cpp +++ b/tests/integration/migration/cassandra/DBRawData.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/DBRawData.hpp b/tests/integration/migration/cassandra/DBRawData.hpp index 595f6ff69..dd28ac9d4 100644 --- a/tests/integration/migration/cassandra/DBRawData.hpp +++ b/tests/integration/migration/cassandra/DBRawData.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleDropTableMigrator.cpp b/tests/integration/migration/cassandra/ExampleDropTableMigrator.cpp index 42c037102..7f5388f38 100644 --- a/tests/integration/migration/cassandra/ExampleDropTableMigrator.cpp +++ b/tests/integration/migration/cassandra/ExampleDropTableMigrator.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleDropTableMigrator.hpp b/tests/integration/migration/cassandra/ExampleDropTableMigrator.hpp index 18453a462..8660438ea 100644 --- a/tests/integration/migration/cassandra/ExampleDropTableMigrator.hpp +++ b/tests/integration/migration/cassandra/ExampleDropTableMigrator.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleLedgerMigrator.cpp b/tests/integration/migration/cassandra/ExampleLedgerMigrator.cpp index 45494c232..cabec9b5a 100644 --- a/tests/integration/migration/cassandra/ExampleLedgerMigrator.cpp +++ b/tests/integration/migration/cassandra/ExampleLedgerMigrator.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleLedgerMigrator.hpp b/tests/integration/migration/cassandra/ExampleLedgerMigrator.hpp index 7be198976..8db567607 100644 --- a/tests/integration/migration/cassandra/ExampleLedgerMigrator.hpp +++ b/tests/integration/migration/cassandra/ExampleLedgerMigrator.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleObjectsMigrator.cpp b/tests/integration/migration/cassandra/ExampleObjectsMigrator.cpp index 2f8aafd2d..c62a52909 100644 --- a/tests/integration/migration/cassandra/ExampleObjectsMigrator.cpp +++ b/tests/integration/migration/cassandra/ExampleObjectsMigrator.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleObjectsMigrator.hpp b/tests/integration/migration/cassandra/ExampleObjectsMigrator.hpp index 6013d7d66..d66399a40 100644 --- a/tests/integration/migration/cassandra/ExampleObjectsMigrator.hpp +++ b/tests/integration/migration/cassandra/ExampleObjectsMigrator.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleTransactionsMigrator.cpp b/tests/integration/migration/cassandra/ExampleTransactionsMigrator.cpp index 32a564fea..3cf8cc9d8 100644 --- a/tests/integration/migration/cassandra/ExampleTransactionsMigrator.cpp +++ b/tests/integration/migration/cassandra/ExampleTransactionsMigrator.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/migration/cassandra/ExampleTransactionsMigrator.hpp b/tests/integration/migration/cassandra/ExampleTransactionsMigrator.hpp index 3701bf61d..82906a4e4 100644 --- a/tests/integration/migration/cassandra/ExampleTransactionsMigrator.hpp +++ b/tests/integration/migration/cassandra/ExampleTransactionsMigrator.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/util/CassandraDBHelper.cpp b/tests/integration/util/CassandraDBHelper.cpp index edf5bd90a..b78452f0a 100644 --- a/tests/integration/util/CassandraDBHelper.cpp +++ b/tests/integration/util/CassandraDBHelper.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above diff --git a/tests/integration/util/CassandraDBHelper.hpp b/tests/integration/util/CassandraDBHelper.hpp index 59f7e96b9..cdb1c1f8a 100644 --- a/tests/integration/util/CassandraDBHelper.hpp +++ b/tests/integration/util/CassandraDBHelper.hpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022-2024, the clio developers. + Copyright (c) 2024, the clio developers. Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above From fbedeff69768b6e9f639e8fc69be1ba210398303 Mon Sep 17 00:00:00 2001 From: nkramer44 Date: Tue, 21 Jan 2025 07:17:54 -0500 Subject: [PATCH 13/31] fix: Remove InvalidHotWallet Error from gateway_balances RPC handler (#1830) Fixes #1825 by removing the check in the gateway_balances RPC handler that returns the RpcInvalidHotWallet error code if one of the addresses supplied in the request's `hotwallet` array does not have a trustline with the `account` from the request. As stated in the original ticket, this change fixes a discrepancy in behavior between Clio and rippled, as rippled does not check for trustline existence when handling gateway_balances RPCs Co-authored-by: Sergey Kuznetsov --- src/rpc/Errors.cpp | 1 - src/rpc/Errors.hpp | 1 - src/rpc/handlers/GatewayBalances.cpp | 4 -- src/web/impl/ErrorHandling.hpp | 1 - src/web/ng/impl/ErrorHandling.cpp | 1 - .../rpc/handlers/GatewayBalancesTests.cpp | 47 ------------------- 6 files changed, 55 deletions(-) diff --git a/src/rpc/Errors.cpp b/src/rpc/Errors.cpp index 16e91ae83..01e5305fb 100644 --- a/src/rpc/Errors.cpp +++ b/src/rpc/Errors.cpp @@ -79,7 +79,6 @@ getErrorInfo(ClioError code) {.code = ClioError::RpcMalformedRequest, .error = "malformedRequest", .message = "Malformed request."}, {.code = ClioError::RpcMalformedOwner, .error = "malformedOwner", .message = "Malformed owner."}, {.code = ClioError::RpcMalformedAddress, .error = "malformedAddress", .message = "Malformed address."}, - {.code = ClioError::RpcInvalidHotWallet, .error = "invalidHotWallet", .message = "Invalid hot wallet."}, {.code = ClioError::RpcUnknownOption, .error = "unknownOption", .message = "Unknown option."}, {.code = ClioError::RpcFieldNotFoundTransaction, .error = "fieldNotFoundTransaction", diff --git a/src/rpc/Errors.hpp b/src/rpc/Errors.hpp index 8322cacf4..4ea7a4ebc 100644 --- a/src/rpc/Errors.hpp +++ b/src/rpc/Errors.hpp @@ -39,7 +39,6 @@ enum class ClioError { RpcMalformedRequest = 5001, RpcMalformedOwner = 5002, RpcMalformedAddress = 5003, - RpcInvalidHotWallet = 5004, RpcUnknownOption = 5005, RpcFieldNotFoundTransaction = 5006, RpcMalformedOracleDocumentId = 5007, diff --git a/src/rpc/handlers/GatewayBalances.cpp b/src/rpc/handlers/GatewayBalances.cpp index 93f89ddeb..5209f1fa7 100644 --- a/src/rpc/handlers/GatewayBalances.cpp +++ b/src/rpc/handlers/GatewayBalances.cpp @@ -145,10 +145,6 @@ GatewayBalancesHandler::process(GatewayBalancesHandler::Input input, Context con if (auto status = std::get_if(&ret)) return Error{*status}; - auto inHotbalances = [&](auto const& hw) { return output.hotBalances.contains(hw); }; - if (not std::ranges::all_of(input.hotWallets, inHotbalances)) - return Error{Status{ClioError::RpcInvalidHotWallet}}; - output.accountID = input.account; output.ledgerHash = ripple::strHex(lgrInfo.hash); output.ledgerIndex = lgrInfo.seq; diff --git a/src/web/impl/ErrorHandling.hpp b/src/web/impl/ErrorHandling.hpp index 5ffbbaa0c..e35126ec4 100644 --- a/src/web/impl/ErrorHandling.hpp +++ b/src/web/impl/ErrorHandling.hpp @@ -88,7 +88,6 @@ public: case rpc::ClioError::RpcMalformedRequest: case rpc::ClioError::RpcMalformedOwner: case rpc::ClioError::RpcMalformedAddress: - case rpc::ClioError::RpcInvalidHotWallet: case rpc::ClioError::RpcFieldNotFoundTransaction: case rpc::ClioError::RpcMalformedOracleDocumentId: case rpc::ClioError::RpcMalformedAuthorizedCredentials: diff --git a/src/web/ng/impl/ErrorHandling.cpp b/src/web/ng/impl/ErrorHandling.cpp index ec6ca4bf6..f8c244289 100644 --- a/src/web/ng/impl/ErrorHandling.cpp +++ b/src/web/ng/impl/ErrorHandling.cpp @@ -102,7 +102,6 @@ ErrorHelper::makeError(rpc::Status const& err) const case rpc::ClioError::RpcMalformedRequest: case rpc::ClioError::RpcMalformedOwner: case rpc::ClioError::RpcMalformedAddress: - case rpc::ClioError::RpcInvalidHotWallet: case rpc::ClioError::RpcFieldNotFoundTransaction: case rpc::ClioError::RpcMalformedOracleDocumentId: case rpc::ClioError::RpcMalformedAuthorizedCredentials: diff --git a/tests/unit/rpc/handlers/GatewayBalancesTests.cpp b/tests/unit/rpc/handlers/GatewayBalancesTests.cpp index a4891ef28..39706cd03 100644 --- a/tests/unit/rpc/handlers/GatewayBalancesTests.cpp +++ b/tests/unit/rpc/handlers/GatewayBalancesTests.cpp @@ -327,53 +327,6 @@ TEST_F(RPCGatewayBalancesHandlerTest, AccountNotFound) }); } -TEST_F(RPCGatewayBalancesHandlerTest, InvalidHotWallet) -{ - auto const seq = 300; - - EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(1); - // return valid ledgerHeader - auto const ledgerHeader = createLedgerHeader(kLEDGER_HASH, seq); - ON_CALL(*backend_, fetchLedgerBySequence(seq, _)).WillByDefault(Return(ledgerHeader)); - - // return valid account - auto const accountKk = ripple::keylet::account(getAccountIdWithString(kACCOUNT)).key; - ON_CALL(*backend_, doFetchLedgerObject(accountKk, seq, _)).WillByDefault(Return(Blob{'f', 'a', 'k', 'e'})); - - // return valid owner dir - auto const ownerDir = createOwnerDirLedgerObject({ripple::uint256{kINDEX2}}, kINDEX1); - auto const ownerDirKk = ripple::keylet::ownerDir(getAccountIdWithString(kACCOUNT)).key; - ON_CALL(*backend_, doFetchLedgerObject(ownerDirKk, seq, _)) - .WillByDefault(Return(ownerDir.getSerializer().peekData())); - EXPECT_CALL(*backend_, doFetchLedgerObject).Times(2); - - // create a valid line, balance is 0 - auto const line1 = createRippleStateLedgerObject("USD", kISSUER, 0, kACCOUNT, 10, kACCOUNT2, 20, kTXN_ID, 123); - std::vector bbs; - bbs.push_back(line1.getSerializer().peekData()); - ON_CALL(*backend_, doFetchLedgerObjects).WillByDefault(Return(bbs)); - EXPECT_CALL(*backend_, doFetchLedgerObjects).Times(1); - - auto const handler = AnyHandler{GatewayBalancesHandler{backend_}}; - runSpawn([&](auto yield) { - auto const output = handler.process( - json::parse(fmt::format( - R"({{ - "account": "{}", - "hotwallet": "{}" - }})", - kACCOUNT, - kACCOUNT2 - )), - Context{yield} - ); - ASSERT_FALSE(output); - auto const err = rpc::makeError(output.result.error()); - EXPECT_EQ(err.at("error").as_string(), "invalidHotWallet"); - EXPECT_EQ(err.at("error_message").as_string(), "Invalid hot wallet."); - }); -} - struct NormalTestBundle { std::string testName; ripple::STObject mockedDir; From 278f7b1b58691248ee4ef2a41146994cf0648375 Mon Sep 17 00:00:00 2001 From: cyan317 <120398799+cindyyan317@users.noreply.github.com> Date: Tue, 21 Jan 2025 14:10:01 +0000 Subject: [PATCH 14/31] feat: Block clio if migration is blocking (#1834) Add: - Block server if migration is blocking - Initialise the migration related table when server starts against empty DB Add MigrationInspectorInterface. server uses inspector to check the migrators status. --- src/app/ClioApplication.cpp | 11 ++ src/migration/CMakeLists.txt | 2 +- src/migration/MigrationInspectorFactory.hpp | 65 +++++++++ src/migration/MigrationInspectorInterface.hpp | 79 +++++++++++ src/migration/MigrationManagerInterface.hpp | 46 +------ src/migration/README.md | 10 +- .../cassandra/CassandraMigrationManager.hpp | 20 ++- src/migration/impl/MigrationInspectorBase.hpp | 121 ++++++++++++++++ src/migration/impl/MigrationManagerBase.hpp | 59 +------- src/migration/impl/MigratorsRegister.hpp | 46 +++++++ tests/common/migration/TestMigrators.hpp | 16 ++- tests/unit/CMakeLists.txt | 2 + .../migration/MigrationInspectorBaseTests.cpp | 130 ++++++++++++++++++ .../MigrationInspectorFactoryTests.cpp | 75 ++++++++++ .../migration/MigrationManagerBaseTests.cpp | 1 - .../unit/migration/MigratorRegisterTests.cpp | 49 +++++-- 16 files changed, 612 insertions(+), 120 deletions(-) create mode 100644 src/migration/MigrationInspectorFactory.hpp create mode 100644 src/migration/MigrationInspectorInterface.hpp create mode 100644 src/migration/impl/MigrationInspectorBase.hpp create mode 100644 tests/unit/migration/MigrationInspectorBaseTests.cpp create mode 100644 tests/unit/migration/MigrationInspectorFactoryTests.cpp diff --git a/src/app/ClioApplication.cpp b/src/app/ClioApplication.cpp index 6a64206f4..393c7b386 100644 --- a/src/app/ClioApplication.cpp +++ b/src/app/ClioApplication.cpp @@ -26,6 +26,7 @@ #include "etl/LoadBalancer.hpp" #include "etl/NetworkValidatedLedgers.hpp" #include "feed/SubscriptionManager.hpp" +#include "migration/MigrationInspectorFactory.hpp" #include "rpc/Counters.hpp" #include "rpc/RPCEngine.hpp" #include "rpc/WorkQueue.hpp" @@ -103,6 +104,16 @@ ClioApplication::run(bool const useNgWebServer) // Interface to the database auto backend = data::makeBackend(config_); + { + auto const migrationInspector = migration::makeMigrationInspector(config_, backend); + // Check if any migration is blocking Clio server starting. + if (migrationInspector->isBlockingClio() and backend->hardFetchLedgerRangeNoThrow()) { + LOG(util::LogService::error()) + << "Existing Migration is blocking Clio, Please complete the database migration first."; + return EXIT_FAILURE; + } + } + // Manages clients subscribed to streams auto subscriptions = feed::SubscriptionManager::makeSubscriptionManager(config_, backend); diff --git a/src/migration/CMakeLists.txt b/src/migration/CMakeLists.txt index 95a18031b..bc4499aa7 100644 --- a/src/migration/CMakeLists.txt +++ b/src/migration/CMakeLists.txt @@ -5,4 +5,4 @@ target_sources( cassandra/impl/ObjectsAdapter.cpp cassandra/impl/TransactionsAdapter.cpp ) -target_link_libraries(clio_migration PRIVATE clio_util clio_etl) +target_link_libraries(clio_migration PRIVATE clio_util clio_data) diff --git a/src/migration/MigrationInspectorFactory.hpp b/src/migration/MigrationInspectorFactory.hpp new file mode 100644 index 000000000..039a5dc76 --- /dev/null +++ b/src/migration/MigrationInspectorFactory.hpp @@ -0,0 +1,65 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "data/BackendInterface.hpp" +#include "migration/MigrationInspectorInterface.hpp" +#include "migration/MigratiorStatus.hpp" +#include "migration/cassandra/CassandraMigrationManager.hpp" +#include "util/Assert.hpp" +#include "util/log/Logger.hpp" +#include "util/newconfig/ConfigDefinition.hpp" + +#include +#include + +#include +#include + +namespace migration { + +/** + * @brief A factory function that creates migration inspector instance and initializes the migration table if needed. + * + * @param config The config. + * @param backend The backend instance. It should be initialized before calling this function. + * @return A shared_ptr instance + */ +inline std::shared_ptr +makeMigrationInspector( + util::config::ClioConfigDefinition const& config, + std::shared_ptr const& backend +) +{ + ASSERT(backend != nullptr, "Backend is not initialized"); + + auto inspector = std::make_shared(backend); + + // Database is empty, we need to initialize the migration table if it is a writeable backend + if (not config.get("read_only") and not backend->hardFetchLedgerRangeNoThrow()) { + migration::MigratorStatus migrated(migration::MigratorStatus::Migrated); + for (auto const& name : inspector->allMigratorsNames()) { + backend->writeMigratorStatus(name, migrated.toString()); + } + } + return inspector; +} + +} // namespace migration diff --git a/src/migration/MigrationInspectorInterface.hpp b/src/migration/MigrationInspectorInterface.hpp new file mode 100644 index 000000000..a3382bc0f --- /dev/null +++ b/src/migration/MigrationInspectorInterface.hpp @@ -0,0 +1,79 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "migration/MigratiorStatus.hpp" + +#include +#include +#include + +namespace migration { + +/** + * @brief The interface for the migration inspector.The Clio server application will use this interface to inspect + * the migration status. + */ +struct MigrationInspectorInterface { + virtual ~MigrationInspectorInterface() = default; + + /** + * @brief Get the status of all the migrators + * @return A vector of tuple, the first element is the migrator's name, the second element is the status of the + */ + virtual std::vector> + allMigratorsStatusPairs() const = 0; + + /** + * @brief Get all registered migrators' names + * + * @return A vector of migrators' names + */ + virtual std::vector + allMigratorsNames() const = 0; + + /** + * @brief Get the status of a migrator by its name + * + * @param name The migrator's name + * @return The status of the migrator + */ + virtual MigratorStatus + getMigratorStatusByName(std::string const& name) const = 0; + + /** + * @brief Get the description of a migrator by its name + * + * @param name The migrator's name + * @return The description of the migrator + */ + virtual std::string + getMigratorDescriptionByName(std::string const& name) const = 0; + + /** + * @brief Return if Clio server is blocked + * + * @return True if Clio server is blocked by migration, false otherwise + */ + virtual bool + isBlockingClio() const = 0; +}; + +} // namespace migration diff --git a/src/migration/MigrationManagerInterface.hpp b/src/migration/MigrationManagerInterface.hpp index 863c336ab..e76bfeb54 100644 --- a/src/migration/MigrationManagerInterface.hpp +++ b/src/migration/MigrationManagerInterface.hpp @@ -19,59 +19,23 @@ #pragma once -#include "migration/MigratiorStatus.hpp" +#include "migration/MigrationInspectorInterface.hpp" #include -#include -#include namespace migration { /** - * @brief The interface for the migration manager. This interface is tend to be implemented for specific database. The - * application layer will use this interface to run the migrations. + * @brief The interface for the migration manager. The migration application layer will use this interface to run the + * migrations. Unlike the MigrationInspectorInterface which only provides the status of migration, this interface + * contains the acutal migration running method. */ -struct MigrationManagerInterface { - virtual ~MigrationManagerInterface() = default; - +struct MigrationManagerInterface : virtual public MigrationInspectorInterface { /** * @brief Run the the migration according to the given migrator's name */ virtual void runMigration(std::string const&) = 0; - - /** - * @brief Get the status of all the migrators - * @return A vector of tuple, the first element is the migrator's name, the second element is the status of the - */ - virtual std::vector> - allMigratorsStatusPairs() const = 0; - - /** - * @brief Get all registered migrators' names - * - * @return A vector of migrators' names - */ - virtual std::vector - allMigratorsNames() const = 0; - - /** - * @brief Get the status of a migrator by its name - * - * @param name The migrator's name - * @return The status of the migrator - */ - virtual MigratorStatus - getMigratorStatusByName(std::string const& name) const = 0; - - /** - * @brief Get the description of a migrator by its name - * - * @param name The migrator's name - * @return The description of the migrator - */ - virtual std::string - getMigratorDescriptionByName(std::string const& name) const = 0; }; } // namespace migration diff --git a/src/migration/README.md b/src/migration/README.md index 087c08024..9c297a894 100644 --- a/src/migration/README.md +++ b/src/migration/README.md @@ -40,9 +40,11 @@ A migrator satisfies the `MigratorSpec`(impl/Spec.hpp) concept. It contains: -- A `name` which will be used to identify the migrator. User will refer this migrator in command-line tool by this name. The name needs to be different with other migrators, otherwise a compilation error will be raised. +- A `kNAME` which will be used to identify the migrator. User will refer this migrator in command-line tool by this name. The name needs to be different with other migrators, otherwise a compilation error will be raised. -- A `description` which is the detail information of the migrator. +- A `kDESCRIPTION` which is the detail information of the migrator. + +- An optional `kCAN_BLOCK_CLIO` which indicates whether the migrator can block the Clio server. If it's absent, the migrator can't block server. If there is a blocking migrator not completed, the Clio server will fail to start. - A static function `runMigration`, it will be called when user run `--migrate name`. It accepts two parameters: backend, which provides the DB operations interface, and cfg, which provides migration-related configuration. Each migrator can have its own configuration under `.migration` session. @@ -65,8 +67,8 @@ Most indexes are based on either ledger states or transactions. We provide the ` If you need to do full scan against other table, you can follow below steps: - Describe the table which needs full scan in a struct. It has to satisfy the `TableSpec`(cassandra/Spec.hpp) concept, containing static member: - Tuple type `Row`, it's the type of each field in a row. The order of types should match what database will return in a row. Key types should come first, followed by other field types sorted in alphabetical order. - - `PARTITION_KEY`, it's the name of the partition key of the table. - - `TABLE_NAME` + - `kPARTITION_KEY`, it's the name of the partition key of the table. + - `kTABLE_NAME` - Inherent from `FullTableScannerAdapterBase`. - Implement `onRowRead`, its parameter is the `Row` we defined. It's the callback function when a row is read. diff --git a/src/migration/cassandra/CassandraMigrationManager.hpp b/src/migration/cassandra/CassandraMigrationManager.hpp index 65b0f2212..a658a8201 100644 --- a/src/migration/cassandra/CassandraMigrationManager.hpp +++ b/src/migration/cassandra/CassandraMigrationManager.hpp @@ -19,20 +19,32 @@ #pragma once +#include "data/BackendInterface.hpp" #include "migration/cassandra/CassandraMigrationBackend.hpp" +#include "migration/impl/MigrationInspectorBase.hpp" #include "migration/impl/MigrationManagerBase.hpp" #include "migration/impl/MigratorsRegister.hpp" -namespace migration::cassandra { +namespace { // Register migrators here // MigratorsRegister template using CassandraSupportedMigrators = migration::impl::MigratorsRegister; -// Register with MigrationBackend which proceeds the migration -using MigrationProcesser = CassandraSupportedMigrators; +// Instantiates with the backend which supports actual migration running +using MigrationProcesser = CassandraSupportedMigrators; + +// Instantiates with backend interface, it doesn't support actual migration. But it can be used to inspect the migrators +// status +using MigrationQuerier = CassandraSupportedMigrators; + +} // namespace + +namespace migration::cassandra { + +using CassandraMigrationInspector = migration::impl::MigrationInspectorBase; -// The Cassandra migration manager using CassandraMigrationManager = migration::impl::MigrationManagerBase; + } // namespace migration::cassandra diff --git a/src/migration/impl/MigrationInspectorBase.hpp b/src/migration/impl/MigrationInspectorBase.hpp new file mode 100644 index 000000000..fe712ec6b --- /dev/null +++ b/src/migration/impl/MigrationInspectorBase.hpp @@ -0,0 +1,121 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "migration/MigrationInspectorInterface.hpp" +#include "migration/MigratiorStatus.hpp" + +#include +#include +#include +#include +#include + +namespace migration::impl { + +/** + * @brief The migration inspector implementation for Cassandra. It will report the migration status for Cassandra + * database. + * + * @tparam SupportedMigrators The migrators resgister that contains all the migrators + */ +template +class MigrationInspectorBase : virtual public MigrationInspectorInterface { +protected: + SupportedMigrators migrators_; + +public: + /** + * @brief Construct a new Cassandra Migration Inspector object + * + * @param backend The backend of the Cassandra database + */ + explicit MigrationInspectorBase(std::shared_ptr backend) + : migrators_{std::move(backend)} + { + } + + /** + * @brief Get the status of all the migrators + * + * @return A vector of tuple, the first element is the migrator's name, the second element is the status of the + * migrator + */ + std::vector> + allMigratorsStatusPairs() const override + { + return migrators_.getMigratorsStatus(); + } + + /** + * @brief Get the status of a migrator by its name + * + * @param name The name of the migrator + * @return The status of the migrator + */ + MigratorStatus + getMigratorStatusByName(std::string const& name) const override + { + return migrators_.getMigratorStatus(name); + } + + /** + * @brief Get all registered migrators' names + * + * @return A vector of string, the names of all the migrators + */ + std::vector + allMigratorsNames() const override + { + auto const names = migrators_.getMigratorNames(); + return std::vector{names.begin(), names.end()}; + } + + /** + * @brief Get the description of a migrator by its name + * + * @param name The name of the migrator + * @return The description of the migrator + */ + std::string + getMigratorDescriptionByName(std::string const& name) const override + { + return migrators_.getMigratorDescription(name); + } + + /** + * @brief Return if there is uncomplete migrator blocking the server + * + * @return True if server is blocked, false otherwise + */ + bool + isBlockingClio() const override + { + return std::ranges::any_of(migrators_.getMigratorNames(), [&](auto const& migrator) { + if (auto canBlock = migrators_.canMigratorBlockClio(migrator); canBlock.has_value() and *canBlock and + migrators_.getMigratorStatus(std::string(migrator)) == MigratorStatus::Status::NotMigrated) { + return true; + } + return false; + }); + } +}; + +} // namespace migration::impl diff --git a/src/migration/impl/MigrationManagerBase.hpp b/src/migration/impl/MigrationManagerBase.hpp index c4f2d61fe..76c21231a 100644 --- a/src/migration/impl/MigrationManagerBase.hpp +++ b/src/migration/impl/MigrationManagerBase.hpp @@ -20,14 +20,12 @@ #pragma once #include "migration/MigrationManagerInterface.hpp" -#include "migration/MigratiorStatus.hpp" +#include "migration/impl/MigrationInspectorBase.hpp" #include "util/newconfig/ObjectView.hpp" #include #include -#include #include -#include namespace migration::impl { @@ -38,8 +36,7 @@ namespace migration::impl { * @tparam SupportedMigrators The migrators resgister that contains all the migrators */ template -class MigrationManagerBase : public MigrationManagerInterface { - SupportedMigrators migrators_; +class MigrationManagerBase : public MigrationManagerInterface, public MigrationInspectorBase { // contains only migration related settings util::config::ObjectView config_; @@ -54,7 +51,7 @@ public: std::shared_ptr backend, util::config::ObjectView config ) - : migrators_{backend}, config_{std::move(config)} + : MigrationInspectorBase{std::move(backend)}, config_{std::move(config)} { } @@ -66,55 +63,7 @@ public: void runMigration(std::string const& name) override { - migrators_.runMigrator(name, config_); - } - - /** - * @brief Get the status of all the migrators - * - * @return A vector of tuple, the first element is the migrator's name, the second element is the status of the - * migrator - */ - std::vector> - allMigratorsStatusPairs() const override - { - return migrators_.getMigratorsStatus(); - } - - /** - * @brief Get the status of a migrator by its name - * - * @param name The name of the migrator - * @return The status of the migrator - */ - MigratorStatus - getMigratorStatusByName(std::string const& name) const override - { - return migrators_.getMigratorStatus(name); - } - - /** - * @brief Get all registered migrators' names - * - * @return A vector of string, the names of all the migrators - */ - std::vector - allMigratorsNames() const override - { - auto const names = migrators_.getMigratorNames(); - return std::vector{names.begin(), names.end()}; - } - - /** - * @brief Get the description of a migrator by its name - * - * @param name The name of the migrator - * @return The description of the migrator - */ - std::string - getMigratorDescriptionByName(std::string const& name) const override - { - return migrators_.getMigratorDescription(name); + this->migrators_.runMigrator(name, config_); } }; diff --git a/src/migration/impl/MigratorsRegister.hpp b/src/migration/impl/MigratorsRegister.hpp index b27479b53..95bbc783e 100644 --- a/src/migration/impl/MigratorsRegister.hpp +++ b/src/migration/impl/MigratorsRegister.hpp @@ -22,6 +22,7 @@ #include "data/BackendInterface.hpp" #include "migration/MigratiorStatus.hpp" #include "migration/impl/Spec.hpp" +#include "util/Assert.hpp" #include "util/Concepts.hpp" #include "util/log/Logger.hpp" #include "util/newconfig/ObjectView.hpp" @@ -30,10 +31,12 @@ #include #include #include +#include #include #include #include #include +#include #include namespace migration::impl { @@ -47,6 +50,11 @@ concept MigrationBackend = requires { requires std::same_as concept BackendMatchAllMigrators = (MigrationBackend && ...); +template +concept HasCanBlockClio = requires(T t) { + { t.kCAN_BLOCK_CLIO }; +}; + /** *@brief The register of migrators. It will dispatch the migration to the corresponding migrator. It also *hold the shared pointer of backend, which is used by the migrators. @@ -81,6 +89,23 @@ class MigratorsRegister { return (T::kNAME == targetName) ? T::kDESCRIPTION : ""; } + template + static constexpr bool + canBlockClioHelper(std::string_view targetName) + { + if (targetName == First::kNAME) { + if constexpr (HasCanBlockClio) { + return First::kCAN_BLOCK_CLIO; + } + return false; + } + if constexpr (sizeof...(Rest) > 0) { + return canBlockClioHelper(targetName); + } + ASSERT(false, "The migrator name is not found"); + std::unreachable(); + } + public: /** * @brief The backend type which is used by the migrators @@ -179,6 +204,27 @@ public: return result.empty() ? "No Description" : result; } } + + /** + * @brief Return if the given migrator can block Clio server + * + * @param name The migrator's name + * @return std::nullopt if the migrator name is not found, or a boolean value indicating whether the migrator is + * blocking Clio server. + */ + std::optional + canMigratorBlockClio(std::string_view name) const + { + if constexpr (sizeof...(MigratorType) == 0) { + return std::nullopt; + } else { + auto const migratiors = getMigratorNames(); + if (std::ranges::find(migratiors, name) == migratiors.end()) + return std::nullopt; + + return canBlockClioHelper(name); + } + } }; } // namespace migration::impl diff --git a/tests/common/migration/TestMigrators.hpp b/tests/common/migration/TestMigrators.hpp index 38131d50d..03fd64cb3 100644 --- a/tests/common/migration/TestMigrators.hpp +++ b/tests/common/migration/TestMigrators.hpp @@ -26,13 +26,10 @@ struct SimpleTestMigrator { using Backend = MockMigrationBackend; static constexpr auto kNAME = "SimpleTestMigrator"; static constexpr auto kDESCRIPTION = "The migrator for version 0 -> 1"; - static void - runMigration(std::shared_ptr, util::config::ObjectView const&) - { - } + static constexpr auto kCAN_BLOCK_CLIO = true; static void - reset() + runMigration(std::shared_ptr, util::config::ObjectView const&) { } }; @@ -45,9 +42,16 @@ struct SimpleTestMigrator2 { runMigration(std::shared_ptr, util::config::ObjectView const&) { } +}; + +struct SimpleTestMigrator3 { + using Backend = MockMigrationBackend; + static constexpr auto kNAME = "SimpleTestMigrator3"; + static constexpr auto kDESCRIPTION = "The migrator for version 3 -> 4"; + static constexpr auto kCAN_BLOCK_CLIO = false; static void - reset() + runMigration(std::shared_ptr, util::config::ObjectView const&) { } }; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index e9df2ad74..62d191277 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -60,6 +60,8 @@ target_sources( migration/cassandra/SpecTests.cpp migration/MigratorRegisterTests.cpp migration/MigratorStatusTests.cpp + migration/MigrationInspectorFactoryTests.cpp + migration/MigrationInspectorBaseTests.cpp migration/MigrationManagerBaseTests.cpp migration/MigrationManagerFactoryTests.cpp migration/SpecTests.cpp diff --git a/tests/unit/migration/MigrationInspectorBaseTests.cpp b/tests/unit/migration/MigrationInspectorBaseTests.cpp new file mode 100644 index 000000000..d070ee6f7 --- /dev/null +++ b/tests/unit/migration/MigrationInspectorBaseTests.cpp @@ -0,0 +1,130 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== +#include "data/BackendInterface.hpp" +#include "migration/MigratiorStatus.hpp" +#include "migration/TestMigrators.hpp" +#include "migration/impl/MigrationManagerBase.hpp" +#include "migration/impl/MigratorsRegister.hpp" +#include "util/MockBackendTestFixture.hpp" +#include "util/MockPrometheus.hpp" + +#include +#include + +#include +#include +#include +#include + +using TestMigratorRegister = + migration::impl::MigratorsRegister; + +using TestCassandramigrationInspector = migration::impl::MigrationInspectorBase; + +struct MigrationInspectorBaseTest : public util::prometheus::WithMockPrometheus, public MockBackendTest { + MigrationInspectorBaseTest() + { + migrationInspector_ = std::make_shared(backend_); + } + +protected: + std::shared_ptr migrationInspector_; +}; + +TEST_F(MigrationInspectorBaseTest, AllStatus) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator", testing::_)).WillOnce(testing::Return("Migrated")); + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator2", testing::_)) + .WillOnce(testing::Return("NotMigrated")); + auto const status = migrationInspector_->allMigratorsStatusPairs(); + EXPECT_EQ(status.size(), 2); + EXPECT_TRUE( + std::ranges::find(status, std::make_tuple("SimpleTestMigrator", migration::MigratorStatus::Migrated)) != + status.end() + ); + EXPECT_TRUE( + std::ranges::find(status, std::make_tuple("SimpleTestMigrator2", migration::MigratorStatus::NotMigrated)) != + status.end() + ); +} + +TEST_F(MigrationInspectorBaseTest, AllNames) +{ + auto const names = migrationInspector_->allMigratorsNames(); + EXPECT_EQ(names.size(), 2); + EXPECT_EQ(names[0], "SimpleTestMigrator"); + EXPECT_EQ(names[1], "SimpleTestMigrator2"); +} + +TEST_F(MigrationInspectorBaseTest, Description) +{ + EXPECT_EQ(migrationInspector_->getMigratorDescriptionByName("unknown"), "No Description"); + EXPECT_EQ( + migrationInspector_->getMigratorDescriptionByName("SimpleTestMigrator"), "The migrator for version 0 -> 1" + ); + EXPECT_EQ( + migrationInspector_->getMigratorDescriptionByName("SimpleTestMigrator2"), "The migrator for version 1 -> 2" + ); +} + +TEST_F(MigrationInspectorBaseTest, getMigratorStatusByName) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator", testing::_)).WillOnce(testing::Return("Migrated")); + EXPECT_EQ(migrationInspector_->getMigratorStatusByName("SimpleTestMigrator"), migration::MigratorStatus::Migrated); + + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator2", testing::_)) + .WillOnce(testing::Return("NotMigrated")); + EXPECT_EQ( + migrationInspector_->getMigratorStatusByName("SimpleTestMigrator2"), migration::MigratorStatus::NotMigrated + ); +} + +TEST_F(MigrationInspectorBaseTest, oneMigratorBlockingClio) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator", testing::_)) + .WillOnce(testing::Return("NotMigrated")); + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator2", testing::_)).Times(0); + + EXPECT_TRUE(migrationInspector_->isBlockingClio()); +} + +TEST_F(MigrationInspectorBaseTest, oneMigratorBlockingClioGetMigrated) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator", testing::_)).WillOnce(testing::Return("Migrated")); + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator2", testing::_)).Times(0); + EXPECT_FALSE(migrationInspector_->isBlockingClio()); +} + +TEST_F(MigrationInspectorBaseTest, noMigratorBlockingClio) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus).Times(0); + + auto const migrations = migration::impl::MigrationInspectorBase< + migration::impl::MigratorsRegister>(backend_); + EXPECT_FALSE(migrations.isBlockingClio()); +} + +TEST_F(MigrationInspectorBaseTest, isBlockingClioWhenNoMigrator) +{ + EXPECT_CALL(*backend_, fetchMigratorStatus).Times(0); + + auto const migrations = + migration::impl::MigrationInspectorBase>(backend_); + EXPECT_FALSE(migrations.isBlockingClio()); +} diff --git a/tests/unit/migration/MigrationInspectorFactoryTests.cpp b/tests/unit/migration/MigrationInspectorFactoryTests.cpp new file mode 100644 index 000000000..9c91a8aca --- /dev/null +++ b/tests/unit/migration/MigrationInspectorFactoryTests.cpp @@ -0,0 +1,75 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "data/Types.hpp" +#include "migration/MigrationInspectorFactory.hpp" +#include "util/MockBackendTestFixture.hpp" +#include "util/MockPrometheus.hpp" +#include "util/newconfig/ConfigDefinition.hpp" +#include "util/newconfig/ConfigValue.hpp" +#include "util/newconfig/Types.hpp" + +#include +#include + +#include + +using namespace testing; + +struct MigrationInspectorFactoryTests : public util::prometheus::WithPrometheus, public MockBackendTest { +protected: + util::config::ClioConfigDefinition const readerConfig_ = util::config::ClioConfigDefinition{ + {"read_only", util::config::ConfigValue{util::config::ConfigType::Boolean}.defaultValue(true)} + }; +}; + +struct MigrationInspectorFactoryTestsDeathTest : public MigrationInspectorFactoryTests {}; + +TEST_F(MigrationInspectorFactoryTestsDeathTest, NullBackend) +{ + EXPECT_DEATH(migration::makeMigrationInspector(readerConfig_, nullptr), ".*"); +} + +TEST_F(MigrationInspectorFactoryTests, NotInitMigrationTableIfReader) +{ + EXPECT_CALL(*backend_, hardFetchLedgerRange).Times(0); + + EXPECT_NE(migration::makeMigrationInspector(readerConfig_, backend_), nullptr); +} + +TEST_F(MigrationInspectorFactoryTests, BackendIsWriterAndDBEmpty) +{ + EXPECT_CALL(*backend_, hardFetchLedgerRange).WillOnce(Return(std::nullopt)); + + util::config::ClioConfigDefinition const writerConfig = util::config::ClioConfigDefinition{ + {"read_only", util::config::ConfigValue{util::config::ConfigType::Boolean}.defaultValue(false)} + }; + EXPECT_NE(migration::makeMigrationInspector(writerConfig, backend_), nullptr); +} + +TEST_F(MigrationInspectorFactoryTests, BackendIsWriterAndDBNotEmpty) +{ + LedgerRange const range{.minSequence = 1, .maxSequence = 5}; + EXPECT_CALL(*backend_, hardFetchLedgerRange).WillOnce(Return(range)); + + util::config::ClioConfigDefinition const writerConfig = util::config::ClioConfigDefinition{ + {"read_only", util::config::ConfigValue{util::config::ConfigType::Boolean}.defaultValue(false)} + }; + EXPECT_NE(migration::makeMigrationInspector(writerConfig, backend_), nullptr); +} diff --git a/tests/unit/migration/MigrationManagerBaseTests.cpp b/tests/unit/migration/MigrationManagerBaseTests.cpp index 9cf8c60bf..3f78c1881 100644 --- a/tests/unit/migration/MigrationManagerBaseTests.cpp +++ b/tests/unit/migration/MigrationManagerBaseTests.cpp @@ -54,7 +54,6 @@ struct MigrationManagerBaseTest : public util::prometheus::WithMockPrometheus, p MigrationManagerBaseTest() { auto mockBackendPtr = backend_.operator std::shared_ptr(); - TestMigratorRegister const migratorRegister(mockBackendPtr); migrationManager = std::make_shared(mockBackendPtr, cfg.getObject("migration")); } }; diff --git a/tests/unit/migration/MigratorRegisterTests.cpp b/tests/unit/migration/MigratorRegisterTests.cpp index 91e8460b3..d2cf72633 100644 --- a/tests/unit/migration/MigratorRegisterTests.cpp +++ b/tests/unit/migration/MigratorRegisterTests.cpp @@ -67,8 +67,8 @@ TEST_F(MigratorRegisterTests, EmptyMigratorRegister) EXPECT_EQ(migratorRegister.getMigratorDescription("unknown"), "No Description"); } -using MultipleMigratorRegister = - migration::impl::MigratorsRegister; +using MultipleMigratorRegister = migration::impl:: + MigratorsRegister; struct MultipleMigratorRegisterTests : public util::prometheus::WithMockPrometheus, public MockMigrationBackendTest { std::optional migratorRegister; @@ -82,11 +82,11 @@ struct MultipleMigratorRegisterTests : public util::prometheus::WithMockPromethe TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenError) { EXPECT_CALL(*backend_, fetchMigratorStatus(testing::_, testing::_)) - .Times(2) + .Times(3) .WillRepeatedly(testing::Return(std::nullopt)); auto const status = migratorRegister->getMigratorsStatus(); - EXPECT_EQ(status.size(), 2); + EXPECT_EQ(status.size(), 3); EXPECT_TRUE( std::ranges::find(status, std::make_tuple("SimpleTestMigrator", migration::MigratorStatus::NotMigrated)) != status.end() @@ -95,16 +95,20 @@ TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenError) std::ranges::find(status, std::make_tuple("SimpleTestMigrator2", migration::MigratorStatus::NotMigrated)) != status.end() ); + EXPECT_TRUE( + std::ranges::find(status, std::make_tuple("SimpleTestMigrator3", migration::MigratorStatus::NotMigrated)) != + status.end() + ); } TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenReturnInvalidStatus) { EXPECT_CALL(*backend_, fetchMigratorStatus(testing::_, testing::_)) - .Times(2) + .Times(3) .WillRepeatedly(testing::Return("Invalid")); auto const status = migratorRegister->getMigratorsStatus(); - EXPECT_EQ(status.size(), 2); + EXPECT_EQ(status.size(), 3); EXPECT_TRUE( std::ranges::find(status, std::make_tuple("SimpleTestMigrator", migration::MigratorStatus::NotMigrated)) != status.end() @@ -113,6 +117,10 @@ TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenReturnInvalidStatus) std::ranges::find(status, std::make_tuple("SimpleTestMigrator2", migration::MigratorStatus::NotMigrated)) != status.end() ); + EXPECT_TRUE( + std::ranges::find(status, std::make_tuple("SimpleTestMigrator3", migration::MigratorStatus::NotMigrated)) != + status.end() + ); } TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenOneMigrated) @@ -120,9 +128,11 @@ TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenOneMigrated) EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator", testing::_)).WillOnce(testing::Return("Migrated")); EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator2", testing::_)) .WillOnce(testing::Return("NotMigrated")); + EXPECT_CALL(*backend_, fetchMigratorStatus("SimpleTestMigrator3", testing::_)) + .WillOnce(testing::Return("NotMigrated")); auto const status = migratorRegister->getMigratorsStatus(); - EXPECT_EQ(status.size(), 2); + EXPECT_EQ(status.size(), 3); EXPECT_TRUE( std::ranges::find(status, std::make_tuple("SimpleTestMigrator", migration::MigratorStatus::Migrated)) != status.end() @@ -131,6 +141,10 @@ TEST_F(MultipleMigratorRegisterTests, GetMigratorsStatusWhenOneMigrated) std::ranges::find(status, std::make_tuple("SimpleTestMigrator2", migration::MigratorStatus::NotMigrated)) != status.end() ); + EXPECT_TRUE( + std::ranges::find(status, std::make_tuple("SimpleTestMigrator3", migration::MigratorStatus::NotMigrated)) != + status.end() + ); } TEST_F(MultipleMigratorRegisterTests, GetMigratorStatus) @@ -158,9 +172,10 @@ TEST_F(MultipleMigratorRegisterTests, GetMigratorStatusWhenError) TEST_F(MultipleMigratorRegisterTests, Names) { auto names = migratorRegister->getMigratorNames(); - EXPECT_EQ(names.size(), 2); + EXPECT_EQ(names.size(), 3); EXPECT_TRUE(std::ranges::find(names, "SimpleTestMigrator") != names.end()); EXPECT_TRUE(std::ranges::find(names, "SimpleTestMigrator2") != names.end()); + EXPECT_TRUE(std::ranges::find(names, "SimpleTestMigrator3") != names.end()); } TEST_F(MultipleMigratorRegisterTests, Description) @@ -181,3 +196,21 @@ TEST_F(MultipleMigratorRegisterTests, MigrateNormalMigrator) EXPECT_CALL(*backend_, writeMigratorStatus("SimpleTestMigrator", "Migrated")).Times(1); EXPECT_NO_THROW(migratorRegister->runMigrator("SimpleTestMigrator", gCfg.getObject("migration"))); } + +TEST_F(MultipleMigratorRegisterTests, canBlock) +{ + auto canBlock = migratorRegister->canMigratorBlockClio("SimpleTestMigrator"); + EXPECT_TRUE(canBlock); + EXPECT_TRUE(*canBlock); + + canBlock = migratorRegister->canMigratorBlockClio("SimpleTestMigrator2"); + EXPECT_TRUE(canBlock); + EXPECT_FALSE(*canBlock); + + canBlock = migratorRegister->canMigratorBlockClio("SimpleTestMigrator3"); + EXPECT_TRUE(canBlock); + EXPECT_FALSE(*canBlock); + + canBlock = migratorRegister->canMigratorBlockClio("NotAMigrator"); + EXPECT_FALSE(canBlock); +} From f9d987951338160f5b4cb1786e10408c6f916221 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 22 Jan 2025 09:46:37 +0000 Subject: [PATCH 15/31] style: clang-tidy auto fixes (#1845) Fixes #1844. Please review and commit clang-tidy fixes. Co-authored-by: kuznetsss <15742918+kuznetsss@users.noreply.github.com> --- src/migration/MigrationInspectorFactory.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/migration/MigrationInspectorFactory.hpp b/src/migration/MigrationInspectorFactory.hpp index 039a5dc76..2baa29e8b 100644 --- a/src/migration/MigrationInspectorFactory.hpp +++ b/src/migration/MigrationInspectorFactory.hpp @@ -54,7 +54,7 @@ makeMigrationInspector( // Database is empty, we need to initialize the migration table if it is a writeable backend if (not config.get("read_only") and not backend->hardFetchLedgerRangeNoThrow()) { - migration::MigratorStatus migrated(migration::MigratorStatus::Migrated); + migration::MigratorStatus const migrated(migration::MigratorStatus::Migrated); for (auto const& name : inspector->allMigratorsNames()) { backend->writeMigratorStatus(name, migrated.toString()); } From 12e6fcc97e191746f98dd8d0f9eb599d62c9ce8e Mon Sep 17 00:00:00 2001 From: cyan317 <120398799+cindyyan317@users.noreply.github.com> Date: Wed, 22 Jan 2025 09:48:13 +0000 Subject: [PATCH 16/31] fix: gateway_balance discrepancy (#1839) Fix https://github.com/XRPLF/clio/issues/1832 rippled code: https://github.com/XRPLF/rippled/blob/develop/src/xrpld/rpc/handlers/GatewayBalances.cpp#L129 --- src/rpc/handlers/GatewayBalances.hpp | 67 ++++++++------- tests/unit/rpc/handlers/BookOffersTests.cpp | 14 +-- .../rpc/handlers/GatewayBalancesTests.cpp | 85 +++++++++++++++---- 3 files changed, 113 insertions(+), 53 deletions(-) diff --git a/src/rpc/handlers/GatewayBalances.hpp b/src/rpc/handlers/GatewayBalances.hpp index 2822c64c7..08a9e97d2 100644 --- a/src/rpc/handlers/GatewayBalances.hpp +++ b/src/rpc/handlers/GatewayBalances.hpp @@ -108,44 +108,51 @@ public: static RpcSpecConstRef spec([[maybe_unused]] uint32_t apiVersion) { - static auto const kHOT_WALLET_VALIDATOR = - validation::CustomValidator{[](boost::json::value const& value, std::string_view key) -> MaybeError { - if (!value.is_string() && !value.is_array()) - return Error{Status{RippledError::rpcINVALID_PARAMS, std::string(key) + "NotStringOrArray"}}; + auto const getHotWalletValidator = [](RippledError errCode) { + return validation::CustomValidator{ + [errCode](boost::json::value const& value, std::string_view key) -> MaybeError { + if (!value.is_string() && !value.is_array()) + return Error{Status{errCode, std::string(key) + "NotStringOrArray"}}; - // wallet needs to be an valid accountID or public key - auto const wallets = value.is_array() ? value.as_array() : boost::json::array{value}; - auto const getAccountID = [](auto const& j) -> std::optional { - if (j.is_string()) { - auto const pk = util::parseBase58Wrapper( - ripple::TokenType::AccountPublic, boost::json::value_to(j) - ); + // wallet needs to be an valid accountID or public key + auto const wallets = value.is_array() ? value.as_array() : boost::json::array{value}; + auto const getAccountID = [](auto const& j) -> std::optional { + if (j.is_string()) { + auto const pk = util::parseBase58Wrapper( + ripple::TokenType::AccountPublic, boost::json::value_to(j) + ); - if (pk) - return ripple::calcAccountID(*pk); + if (pk) + return ripple::calcAccountID(*pk); - return util::parseBase58Wrapper(boost::json::value_to(j)); + return util::parseBase58Wrapper(boost::json::value_to(j)); + } + + return {}; + }; + + for (auto const& wallet : wallets) { + if (!getAccountID(wallet)) + return Error{Status{errCode, std::string(key) + "Malformed"}}; } - return {}; - }; - - for (auto const& wallet : wallets) { - if (!getAccountID(wallet)) - return Error{Status{RippledError::rpcINVALID_PARAMS, std::string(key) + "Malformed"}}; + return MaybeError{}; } - - return MaybeError{}; - }}; - - static auto const kRPC_SPEC = RpcSpec{ - {JS(account), validation::Required{}, validation::CustomValidators::accountValidator}, - {JS(ledger_hash), validation::CustomValidators::uint256HexStringValidator}, - {JS(ledger_index), validation::CustomValidators::ledgerIndexValidator}, - {JS(hotwallet), kHOT_WALLET_VALIDATOR} + }; }; - return kRPC_SPEC; + static auto const kSPEC_COMMON = RpcSpec{ + {JS(account), validation::Required{}, validation::CustomValidators::accountValidator}, + {JS(ledger_hash), validation::CustomValidators::uint256HexStringValidator}, + {JS(ledger_index), validation::CustomValidators::ledgerIndexValidator} + }; + + auto static const kSPEC_V1 = + RpcSpec{kSPEC_COMMON, {{JS(hotwallet), getHotWalletValidator(ripple::rpcINVALID_HOTWALLET)}}}; + auto static const kSPEC_V2 = + RpcSpec{kSPEC_COMMON, {{JS(hotwallet), getHotWalletValidator(ripple::rpcINVALID_PARAMS)}}}; + + return apiVersion == 1 ? kSPEC_V1 : kSPEC_V2; } /** diff --git a/tests/unit/rpc/handlers/BookOffersTests.cpp b/tests/unit/rpc/handlers/BookOffersTests.cpp index 88d6f3bed..88d3056de 100644 --- a/tests/unit/rpc/handlers/BookOffersTests.cpp +++ b/tests/unit/rpc/handlers/BookOffersTests.cpp @@ -68,6 +68,13 @@ constexpr auto kPAYS20_XRP_GETS10_USD_BOOK_DIR = "7B1767D41DBCE79D9585CF9D0262A5 // transfer rate x2 constexpr auto kTRANSFER_RATE_X2 = 2000000000; +struct ParameterTestBundle { + std::string testName; + std::string testJson; + std::string expectedError; + std::string expectedErrorMessage; +}; + } // namespace using namespace rpc; @@ -81,13 +88,6 @@ struct RPCBookOffersHandlerTest : HandlerBaseTest { } }; -struct ParameterTestBundle { - std::string testName; - std::string testJson; - std::string expectedError; - std::string expectedErrorMessage; -}; - struct RPCBookOffersParameterTest : RPCBookOffersHandlerTest, WithParamInterface {}; TEST_P(RPCBookOffersParameterTest, CheckError) diff --git a/tests/unit/rpc/handlers/GatewayBalancesTests.cpp b/tests/unit/rpc/handlers/GatewayBalancesTests.cpp index 39706cd03..d9e319be1 100644 --- a/tests/unit/rpc/handlers/GatewayBalancesTests.cpp +++ b/tests/unit/rpc/handlers/GatewayBalancesTests.cpp @@ -60,6 +60,13 @@ constexpr auto kINDEX1 = "1B8590C01B0006EDFA9ED60296DD052DC5E90F99659B25014D08E1 constexpr auto kINDEX2 = "E6DBAFC99223B42257915A63DFC6B0C032D4070F9A574B255AD97466726FC321"; constexpr auto kTXN_ID = "E3FE6EA3D48F0C2B639448020EA4F03D4F4F8FFDB243A852A0F59177921B4879"; +struct ParameterTestBundle { + std::string testName; + std::string testJson; + std::string expectedError; + std::string expectedErrorMessage; + std::uint32_t apiVersion = 1u; +}; } // namespace struct RPCGatewayBalancesHandlerTest : HandlerBaseTest { @@ -69,13 +76,6 @@ struct RPCGatewayBalancesHandlerTest : HandlerBaseTest { } }; -struct ParameterTestBundle { - std::string testName; - std::string testJson; - std::string expectedError; - std::string expectedErrorMessage; -}; - struct ParameterTest : public RPCGatewayBalancesHandlerTest, public WithParamInterface {}; TEST_P(ParameterTest, CheckError) @@ -83,7 +83,8 @@ TEST_P(ParameterTest, CheckError) auto bundle = GetParam(); auto const handler = AnyHandler{GatewayBalancesHandler{backend_}}; runSpawn([&](auto yield) { - auto const output = handler.process(json::parse(bundle.testJson), Context{yield}); + auto const output = + handler.process(json::parse(bundle.testJson), Context{.yield = yield, .apiVersion = bundle.apiVersion}); ASSERT_FALSE(output); auto const err = rpc::makeError(output.result.error()); EXPECT_EQ(err.at("error").as_string(), bundle.expectedError); @@ -155,7 +156,55 @@ generateParameterTestBundles() .expectedErrorMessage = "ledger_hashNotString" }, ParameterTestBundle{ - .testName = "WalletsNotStringOrArray", + .testName = "WalletsNotStringOrArrayV1", + .testJson = fmt::format( + R"({{ + "account": "{}", + "hotwallet": 12 + }})", + kACCOUNT + ), + .expectedError = "invalidHotWallet", + .expectedErrorMessage = "hotwalletNotStringOrArray" + }, + ParameterTestBundle{ + .testName = "WalletsNotStringAccountV1", + .testJson = fmt::format( + R"({{ + "account": "{}", + "hotwallet": [12] + }})", + kACCOUNT + ), + .expectedError = "invalidHotWallet", + .expectedErrorMessage = "hotwalletMalformed" + }, + ParameterTestBundle{ + .testName = "WalletsInvalidAccountV1", + .testJson = fmt::format( + R"({{ + "account": "{}", + "hotwallet": ["12"] + }})", + kACCOUNT + ), + .expectedError = "invalidHotWallet", + .expectedErrorMessage = "hotwalletMalformed" + }, + ParameterTestBundle{ + .testName = "WalletInvalidAccountV1", + .testJson = fmt::format( + R"({{ + "account": "{}", + "hotwallet": "12" + }})", + kACCOUNT + ), + .expectedError = "invalidHotWallet", + .expectedErrorMessage = "hotwalletMalformed" + }, + ParameterTestBundle{ + .testName = "WalletsNotStringOrArrayV2", .testJson = fmt::format( R"({{ "account": "{}", @@ -164,10 +213,11 @@ generateParameterTestBundles() kACCOUNT ), .expectedError = "invalidParams", - .expectedErrorMessage = "hotwalletNotStringOrArray" + .expectedErrorMessage = "hotwalletNotStringOrArray", + .apiVersion = 2u }, ParameterTestBundle{ - .testName = "WalletsNotStringAccount", + .testName = "WalletsNotStringAccountV2", .testJson = fmt::format( R"({{ "account": "{}", @@ -176,10 +226,11 @@ generateParameterTestBundles() kACCOUNT ), .expectedError = "invalidParams", - .expectedErrorMessage = "hotwalletMalformed" + .expectedErrorMessage = "hotwalletMalformed", + .apiVersion = 2u }, ParameterTestBundle{ - .testName = "WalletsInvalidAccount", + .testName = "WalletsInvalidAccountV2", .testJson = fmt::format( R"({{ "account": "{}", @@ -188,10 +239,11 @@ generateParameterTestBundles() kACCOUNT ), .expectedError = "invalidParams", - .expectedErrorMessage = "hotwalletMalformed" + .expectedErrorMessage = "hotwalletMalformed", + .apiVersion = 2u }, ParameterTestBundle{ - .testName = "WalletInvalidAccount", + .testName = "WalletInvalidAccountV2", .testJson = fmt::format( R"({{ "account": "{}", @@ -200,7 +252,8 @@ generateParameterTestBundles() kACCOUNT ), .expectedError = "invalidParams", - .expectedErrorMessage = "hotwalletMalformed" + .expectedErrorMessage = "hotwalletMalformed", + .apiVersion = 2u }, }; } From 957028699b8869ad4bc6907bfeeecdda6b7cfcb2 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 22 Jan 2025 13:09:16 +0000 Subject: [PATCH 17/31] feat: Graceful shutdown (#1801) Fixes #442. --- src/app/CMakeLists.txt | 2 +- src/app/ClioApplication.cpp | 8 + src/app/ClioApplication.hpp | 2 + src/app/Stopper.cpp | 52 +++++ src/app/Stopper.hpp | 118 +++++++++++ src/data/BackendInterface.hpp | 6 + src/data/CassandraBackend.hpp | 9 +- src/etl/ETLService.hpp | 29 ++- src/etl/LoadBalancer.cpp | 11 + src/etl/LoadBalancer.hpp | 24 ++- src/etl/Source.hpp | 9 + src/etl/impl/SourceImpl.hpp | 6 + src/etl/impl/SubscriptionSource.cpp | 86 ++++---- src/etl/impl/SubscriptionSource.hpp | 14 +- src/feed/SubscriptionManager.hpp | 9 + src/feed/SubscriptionManagerInterface.hpp | 6 + src/util/CMakeLists.txt | 1 + src/util/StopHelper.cpp | 46 +++++ src/util/StopHelper.hpp | 54 +++++ src/web/ng/Server.cpp | 40 +++- src/web/ng/Server.hpp | 21 +- src/web/ng/impl/ConnectionHandler.cpp | 65 +++++- src/web/ng/impl/ConnectionHandler.hpp | 23 ++- src/web/ng/impl/HttpConnection.hpp | 8 + src/web/ng/impl/WsConnection.hpp | 8 + tests/common/util/AsioContextTestFixture.hpp | 45 +++++ tests/common/util/MockBackend.hpp | 2 + tests/common/util/MockSource.hpp | 8 +- tests/common/util/MockSubscriptionManager.hpp | 2 + tests/common/web/ng/MockConnection.hpp | 2 +- .../common/web/ng/impl/MockHttpConnection.hpp | 2 +- tests/common/web/ng/impl/MockWsConnection.hpp | 2 +- tests/unit/CMakeLists.txt | 2 + tests/unit/app/StopperTests.cpp | 116 +++++++++++ tests/unit/etl/LoadBalancerTests.cpp | 9 + tests/unit/etl/SourceImplTests.cpp | 10 +- tests/unit/etl/SubscriptionSourceTests.cpp | 191 +++++++++--------- tests/unit/util/StopHelperTests.cpp | 62 ++++++ tests/unit/web/ng/ServerTests.cpp | 50 ++++- .../web/ng/impl/ConnectionHandlerTests.cpp | 78 ++++++- tests/unit/web/ng/impl/WsConnectionTests.cpp | 26 +++ 41 files changed, 1073 insertions(+), 191 deletions(-) create mode 100644 src/app/Stopper.cpp create mode 100644 src/app/Stopper.hpp create mode 100644 src/util/StopHelper.cpp create mode 100644 src/util/StopHelper.hpp create mode 100644 tests/unit/app/StopperTests.cpp create mode 100644 tests/unit/util/StopHelperTests.cpp diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 0559fa49c..40cbbbc14 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -1,4 +1,4 @@ add_library(clio_app) -target_sources(clio_app PRIVATE CliArgs.cpp ClioApplication.cpp WebHandlers.cpp) +target_sources(clio_app PRIVATE CliArgs.cpp ClioApplication.cpp Stopper.cpp WebHandlers.cpp) target_link_libraries(clio_app PUBLIC clio_etl clio_etlng clio_feed clio_web clio_rpc clio_migration) diff --git a/src/app/ClioApplication.cpp b/src/app/ClioApplication.cpp index 393c7b386..4056306f4 100644 --- a/src/app/ClioApplication.cpp +++ b/src/app/ClioApplication.cpp @@ -19,6 +19,7 @@ #include "app/ClioApplication.hpp" +#include "app/Stopper.hpp" #include "app/WebHandlers.hpp" #include "data/AmendmentCenter.hpp" #include "data/BackendFactory.hpp" @@ -45,6 +46,8 @@ #include "web/ng/Server.hpp" #include +#include +#include #include #include @@ -84,6 +87,7 @@ ClioApplication::ClioApplication(util::config::ClioConfigDefinition const& confi { LOG(util::LogService::info()) << "Clio version: " << util::build::getClioFullVersionString(); PrometheusService::init(config); + signalsHandler_.subscribeToStop([this]() { appStopper_.stop(); }); } int @@ -169,6 +173,10 @@ ClioApplication::run(bool const useNgWebServer) return EXIT_FAILURE; } + appStopper_.setOnStop( + Stopper::makeOnStopCallback(httpServer.value(), *balancer, *etl, *subscriptions, *backend, ioc) + ); + // Blocks until stopped. // When stopped, shared_ptrs fall out of scope // Calls destructors on all resources, and destructs in order diff --git a/src/app/ClioApplication.hpp b/src/app/ClioApplication.hpp index 65b8f5f81..1eac02aae 100644 --- a/src/app/ClioApplication.hpp +++ b/src/app/ClioApplication.hpp @@ -19,6 +19,7 @@ #pragma once +#include "app/Stopper.hpp" #include "util/SignalsHandler.hpp" #include "util/newconfig/ConfigDefinition.hpp" @@ -30,6 +31,7 @@ namespace app { class ClioApplication { util::config::ClioConfigDefinition const& config_; util::SignalsHandler signalsHandler_; + Stopper appStopper_; public: /** diff --git a/src/app/Stopper.cpp b/src/app/Stopper.cpp new file mode 100644 index 000000000..90da5cde3 --- /dev/null +++ b/src/app/Stopper.cpp @@ -0,0 +1,52 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "app/Stopper.hpp" + +#include + +#include +#include +#include + +namespace app { + +Stopper::~Stopper() +{ + if (worker_.joinable()) + worker_.join(); +} + +void +Stopper::setOnStop(std::function cb) +{ + boost::asio::spawn(ctx_, std::move(cb)); +} + +void +Stopper::stop() +{ + // Do nothing if worker_ is already running + if (worker_.joinable()) + return; + + worker_ = std::thread{[this]() { ctx_.run(); }}; +} + +} // namespace app diff --git a/src/app/Stopper.hpp b/src/app/Stopper.hpp new file mode 100644 index 000000000..b4faa1377 --- /dev/null +++ b/src/app/Stopper.hpp @@ -0,0 +1,118 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include "data/BackendInterface.hpp" +#include "etl/ETLService.hpp" +#include "etl/LoadBalancer.hpp" +#include "feed/SubscriptionManagerInterface.hpp" +#include "util/CoroutineGroup.hpp" +#include "util/log/Logger.hpp" +#include "web/ng/Server.hpp" + +#include +#include +#include + +#include +#include + +namespace app { + +/** + * @brief Application stopper class. On stop it will create a new thread to run all the shutdown tasks. + */ +class Stopper { + boost::asio::io_context ctx_; + std::thread worker_; + +public: + /** + * @brief Destroy the Stopper object + */ + ~Stopper(); + + /** + * @brief Set the callback to be called when the application is stopped. + * + * @param cb The callback to be called on application stop. + */ + void + setOnStop(std::function cb); + + /** + * @brief Stop the application and run the shutdown tasks. + */ + void + stop(); + + /** + * @brief Create a callback to be called on application stop. + * + * @param server The server to stop. + * @param balancer The load balancer to stop. + * @param etl The ETL service to stop. + * @param subscriptions The subscription manager to stop. + * @param backend The backend to stop. + * @param ioc The io_context to stop. + * @return The callback to be called on application stop. + */ + template < + web::ng::SomeServer ServerType, + etl::SomeLoadBalancer LoadBalancerType, + etl::SomeETLService ETLServiceType> + static std::function + makeOnStopCallback( + ServerType& server, + LoadBalancerType& balancer, + ETLServiceType& etl, + feed::SubscriptionManagerInterface& subscriptions, + data::BackendInterface& backend, + boost::asio::io_context& ioc + ) + { + return [&](boost::asio::yield_context yield) { + util::CoroutineGroup coroutineGroup{yield}; + coroutineGroup.spawn(yield, [&server](auto innerYield) { + server.stop(innerYield); + LOG(util::LogService::info()) << "Server stopped"; + }); + coroutineGroup.spawn(yield, [&balancer](auto innerYield) { + balancer.stop(innerYield); + LOG(util::LogService::info()) << "LoadBalancer stopped"; + }); + coroutineGroup.asyncWait(yield); + + etl.stop(); + LOG(util::LogService::info()) << "ETL stopped"; + + subscriptions.stop(); + LOG(util::LogService::info()) << "SubscriptionManager stopped"; + + backend.waitForWritesToFinish(); + LOG(util::LogService::info()) << "Backend writes finished"; + + ioc.stop(); + LOG(util::LogService::info()) << "io_context stopped"; + }; + } +}; + +} // namespace app diff --git a/src/data/BackendInterface.hpp b/src/data/BackendInterface.hpp index 2d7e3ef61..48771bae1 100644 --- a/src/data/BackendInterface.hpp +++ b/src/data/BackendInterface.hpp @@ -683,6 +683,12 @@ public: bool finishWrites(std::uint32_t ledgerSequence); + /** + * @brief Wait for all pending writes to finish. + */ + virtual void + waitForWritesToFinish() = 0; + /** * @brief Mark the migration status of a migrator as Migrated in the database * diff --git a/src/data/CassandraBackend.hpp b/src/data/CassandraBackend.hpp index ba8aacd7f..71e9ecf59 100644 --- a/src/data/CassandraBackend.hpp +++ b/src/data/CassandraBackend.hpp @@ -188,11 +188,16 @@ public: return {txns, {}}; } + void + waitForWritesToFinish() override + { + executor_.sync(); + } + bool doFinishWrites() override { - // wait for other threads to finish their writes - executor_.sync(); + waitForWritesToFinish(); if (!range_) { executor_.writeSync(schema_->updateLedgerRange, ledgerSequence_, false, ledgerSequence_); diff --git a/src/etl/ETLService.hpp b/src/etl/ETLService.hpp index 1dbbd94c6..f0e2a2fc3 100644 --- a/src/etl/ETLService.hpp +++ b/src/etl/ETLService.hpp @@ -42,6 +42,7 @@ #include #include +#include #include #include #include @@ -58,6 +59,16 @@ struct NFTsData; */ namespace etl { +/** + * @brief A tag class to help identify ETLService in templated code. + */ +struct ETLServiceTag { + virtual ~ETLServiceTag() = default; +}; + +template +concept SomeETLService = std::derived_from; + /** * @brief This class is responsible for continuously extracting data from a p2p node, and writing that data to the * databases. @@ -71,7 +82,7 @@ namespace etl { * the others will fall back to monitoring/publishing. In this sense, this class dynamically transitions from monitoring * to writing and from writing to monitoring, based on the activity of other processes running on different machines. */ -class ETLService { +class ETLService : public ETLServiceTag { // TODO: make these template parameters in ETLService using LoadBalancerType = LoadBalancer; using DataPipeType = etl::impl::ExtractionDataPipe; @@ -159,10 +170,20 @@ public: /** * @brief Stops components and joins worker thread. */ - ~ETLService() + ~ETLService() override { - LOG(log_.info()) << "onStop called"; - LOG(log_.debug()) << "Stopping Reporting ETL"; + if (not state_.isStopping) + stop(); + } + + /** + * @brief Stop the ETL service. + * @note This method blocks until the ETL service has stopped. + */ + void + stop() + { + LOG(log_.info()) << "Stop called"; state_.isStopping = true; cacheLoader_.stop(); diff --git a/src/etl/LoadBalancer.cpp b/src/etl/LoadBalancer.cpp index 6e961933c..7dc0d1925 100644 --- a/src/etl/LoadBalancer.cpp +++ b/src/etl/LoadBalancer.cpp @@ -26,6 +26,7 @@ #include "feed/SubscriptionManagerInterface.hpp" #include "rpc/Errors.hpp" #include "util/Assert.hpp" +#include "util/CoroutineGroup.hpp" #include "util/Random.hpp" #include "util/ResponseExpirationCache.hpp" #include "util/log/Logger.hpp" @@ -336,6 +337,16 @@ LoadBalancer::getETLState() noexcept return etlState_; } +void +LoadBalancer::stop(boost::asio::yield_context yield) +{ + util::CoroutineGroup group{yield}; + std::ranges::for_each(sources_, [&group, yield](auto& source) { + group.spawn(yield, [&source](boost::asio::yield_context innerYield) { source->stop(innerYield); }); + }); + group.asyncWait(yield); +} + void LoadBalancer::chooseForwardingSource() { diff --git a/src/etl/LoadBalancer.hpp b/src/etl/LoadBalancer.hpp index 60f56a487..cfe01d019 100644 --- a/src/etl/LoadBalancer.hpp +++ b/src/etl/LoadBalancer.hpp @@ -41,6 +41,7 @@ #include #include +#include #include #include #include @@ -51,6 +52,16 @@ namespace etl { +/** + * @brief A tag class to help identify LoadBalancer in templated code. + */ +struct LoadBalancerTag { + virtual ~LoadBalancerTag() = default; +}; + +template +concept SomeLoadBalancer = std::derived_from; + /** * @brief This class is used to manage connections to transaction processing processes. * @@ -58,7 +69,7 @@ namespace etl { * which ledgers have been validated by the network, and the range of ledgers each etl source has). This class also * allows requests for ledger data to be load balanced across all possible ETL sources. */ -class LoadBalancer { +class LoadBalancer : public LoadBalancerTag { public: using RawLedgerObjectType = org::xrpl::rpc::v1::RawLedgerObject; using GetLedgerResponseType = org::xrpl::rpc::v1::GetLedgerResponse; @@ -132,7 +143,7 @@ public: SourceFactory sourceFactory = makeSource ); - ~LoadBalancer(); + ~LoadBalancer() override; /** * @brief Load the initial ledger, writing data to the queue. @@ -203,6 +214,15 @@ public: std::optional getETLState() noexcept; + /** + * @brief Stop the load balancer. This will stop all subscription sources. + * @note This function will asynchronously wait for all sources to stop. + * + * @param yield The coroutine context + */ + void + stop(boost::asio::yield_context yield); + private: /** * @brief Execute a function on a randomly selected source. diff --git a/src/etl/Source.hpp b/src/etl/Source.hpp index e50849720..91d9bf817 100644 --- a/src/etl/Source.hpp +++ b/src/etl/Source.hpp @@ -65,6 +65,15 @@ public: virtual void run() = 0; + /** + * @brief Stop Source. + * @note This method will asynchronously wait for source to be stopped. + * + * @param yield The coroutine context. + */ + virtual void + stop(boost::asio::yield_context yield) = 0; + /** * @brief Check if source is connected * diff --git a/src/etl/impl/SourceImpl.hpp b/src/etl/impl/SourceImpl.hpp index 7302bfa4c..3f2f25a33 100644 --- a/src/etl/impl/SourceImpl.hpp +++ b/src/etl/impl/SourceImpl.hpp @@ -102,6 +102,12 @@ public: subscriptionSource_->run(); } + void + stop(boost::asio::yield_context yield) final + { + subscriptionSource_->stop(yield); + } + /** * @brief Check if source is connected * diff --git a/src/etl/impl/SubscriptionSource.cpp b/src/etl/impl/SubscriptionSource.cpp index 1684cd23f..0dd081e23 100644 --- a/src/etl/impl/SubscriptionSource.cpp +++ b/src/etl/impl/SubscriptionSource.cpp @@ -49,7 +49,6 @@ #include #include #include -#include #include #include #include @@ -92,15 +91,6 @@ SubscriptionSource::SubscriptionSource( .setConnectionTimeout(wsTimeout_); } -SubscriptionSource::~SubscriptionSource() -{ - stop(); - retry_.cancel(); - - if (runFuture_.valid()) - runFuture_.wait(); -} - void SubscriptionSource::run() { @@ -157,59 +147,53 @@ SubscriptionSource::validatedRange() const } void -SubscriptionSource::stop() +SubscriptionSource::stop(boost::asio::yield_context yield) { stop_ = true; + stopHelper_.asyncWaitForStop(yield); } void SubscriptionSource::subscribe() { - runFuture_ = boost::asio::spawn( - strand_, - [this, _ = boost::asio::make_work_guard(strand_)](boost::asio::yield_context yield) { - auto connection = wsConnectionBuilder_.connect(yield); - if (not connection) { - handleError(connection.error(), yield); - return; - } - + boost::asio::spawn(strand_, [this, _ = boost::asio::make_work_guard(strand_)](boost::asio::yield_context yield) { + if (auto connection = wsConnectionBuilder_.connect(yield); connection) { wsConnection_ = std::move(connection).value(); + } else { + handleError(connection.error(), yield); + return; + } - auto const& subscribeCommand = getSubscribeCommandJson(); - auto const writeErrorOpt = wsConnection_->write(subscribeCommand, yield, wsTimeout_); - if (writeErrorOpt) { - handleError(writeErrorOpt.value(), yield); + auto const& subscribeCommand = getSubscribeCommandJson(); + + if (auto const writeErrorOpt = wsConnection_->write(subscribeCommand, yield, wsTimeout_); writeErrorOpt) { + handleError(writeErrorOpt.value(), yield); + return; + } + + isConnected_ = true; + LOG(log_.info()) << "Connected"; + onConnect_(); + + retry_.reset(); + + while (!stop_) { + auto const message = wsConnection_->read(yield, wsTimeout_); + if (not message) { + handleError(message.error(), yield); return; } - isConnected_ = true; - LOG(log_.info()) << "Connected"; - onConnect_(); - - retry_.reset(); - - while (!stop_) { - auto const message = wsConnection_->read(yield, wsTimeout_); - if (not message) { - handleError(message.error(), yield); - return; - } - - auto const handleErrorOpt = handleMessage(message.value()); - if (handleErrorOpt) { - handleError(handleErrorOpt.value(), yield); - return; - } + if (auto const handleErrorOpt = handleMessage(message.value()); handleErrorOpt) { + handleError(handleErrorOpt.value(), yield); + return; } - // Close the connection - handleError( - util::requests::RequestError{"Subscription source stopped", boost::asio::error::operation_aborted}, - yield - ); - }, - boost::asio::use_future - ); + } + // Close the connection + handleError( + util::requests::RequestError{"Subscription source stopped", boost::asio::error::operation_aborted}, yield + ); + }); } std::optional @@ -299,6 +283,8 @@ SubscriptionSource::handleError(util::requests::RequestError const& error, boost logError(error); if (not stop_) { retry_.retry([this] { subscribe(); }); + } else { + stopHelper_.readyToStop(); } } diff --git a/src/etl/impl/SubscriptionSource.hpp b/src/etl/impl/SubscriptionSource.hpp index 77bdf4b0e..d57cb76b6 100644 --- a/src/etl/impl/SubscriptionSource.hpp +++ b/src/etl/impl/SubscriptionSource.hpp @@ -24,6 +24,7 @@ #include "feed/SubscriptionManagerInterface.hpp" #include "util/Mutex.hpp" #include "util/Retry.hpp" +#include "util/StopHelper.hpp" #include "util/log/Logger.hpp" #include "util/prometheus/Gauge.hpp" #include "util/requests/Types.hpp" @@ -39,7 +40,6 @@ #include #include #include -#include #include #include #include @@ -50,6 +50,7 @@ namespace etl::impl { /** * @brief This class is used to subscribe to a source of ledger data and forward it to the subscription manager. + * @note This class is safe to delete only if io_context is stopped. */ class SubscriptionSource { public: @@ -89,7 +90,7 @@ private: std::reference_wrapper lastMessageTimeSecondsSinceEpoch_; - std::future runFuture_; + util::StopHelper stopHelper_; static constexpr std::chrono::seconds kWS_TIMEOUT{30}; static constexpr std::chrono::seconds kRETRY_MAX_DELAY{30}; @@ -124,13 +125,6 @@ public: std::chrono::steady_clock::duration const retryDelay = SubscriptionSource::kRETRY_DELAY ); - /** - * @brief Destroy the Subscription Source object - * - * @note This will block to wait for all the async operations to complete. io_context must be still running - */ - ~SubscriptionSource(); - /** * @brief Run the source */ @@ -192,7 +186,7 @@ public: * @brief Stop the source. The source will complete already scheduled operations but will not schedule new ones */ void - stop(); + stop(boost::asio::yield_context yield); private: void diff --git a/src/feed/SubscriptionManager.hpp b/src/feed/SubscriptionManager.hpp index 096b7fb2d..d839a672f 100644 --- a/src/feed/SubscriptionManager.hpp +++ b/src/feed/SubscriptionManager.hpp @@ -115,6 +115,15 @@ public: * @brief Destructor of the SubscriptionManager object. It will block until all running jobs finished. */ ~SubscriptionManager() override + { + stop(); + } + + /** + * @brief Stop the SubscriptionManager and wait for all jobs to finish. + */ + void + stop() override { ctx_.stop(); ctx_.join(); diff --git a/src/feed/SubscriptionManagerInterface.hpp b/src/feed/SubscriptionManagerInterface.hpp index c627bc53c..c0efd7f91 100644 --- a/src/feed/SubscriptionManagerInterface.hpp +++ b/src/feed/SubscriptionManagerInterface.hpp @@ -45,6 +45,12 @@ class SubscriptionManagerInterface { public: virtual ~SubscriptionManagerInterface() = default; + /** + * @brief Stop the SubscriptionManager and wait for all jobs to finish. + */ + virtual void + stop() = 0; + /** * @brief Subscribe to the book changes feed. * @param subscriber diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 8c28fdf95..32e792e80 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -22,6 +22,7 @@ target_sources( requests/impl/SslContext.cpp ResponseExpirationCache.cpp SignalsHandler.cpp + StopHelper.cpp Taggable.cpp TerminationHandler.cpp TimeUtils.cpp diff --git a/src/util/StopHelper.cpp b/src/util/StopHelper.cpp new file mode 100644 index 000000000..928773d09 --- /dev/null +++ b/src/util/StopHelper.cpp @@ -0,0 +1,46 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "util/StopHelper.hpp" + +#include +#include + +#include + +namespace util { + +void +StopHelper::readyToStop() +{ + onStopReady_(); + *stopped_ = true; +} + +void +StopHelper::asyncWaitForStop(boost::asio::yield_context yield) +{ + boost::asio::steady_timer timer{yield.get_executor(), std::chrono::steady_clock::duration::max()}; + onStopReady_.connect([&timer]() { timer.cancel(); }); + boost::system::error_code error; + if (!*stopped_) + timer.async_wait(yield[error]); +} + +} // namespace util diff --git a/src/util/StopHelper.hpp b/src/util/StopHelper.hpp new file mode 100644 index 000000000..226352b46 --- /dev/null +++ b/src/util/StopHelper.hpp @@ -0,0 +1,54 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2024, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include +#include +#include + +#include +#include + +namespace util { + +/** + * @brief Helper class to stop a class asynchronously. + */ +class StopHelper { + boost::signals2::signal onStopReady_; + std::unique_ptr stopped_ = std::make_unique(false); + +public: + /** + * @brief Notify that the class is ready to stop. + */ + void + readyToStop(); + + /** + * @brief Wait for the class to stop. + * + * @param yield The coroutine context + */ + void + asyncWaitForStop(boost::asio::yield_context yield); +}; + +} // namespace util diff --git a/src/web/ng/Server.cpp b/src/web/ng/Server.cpp index 11f78c176..70ba28ba3 100644 --- a/src/web/ng/Server.cpp +++ b/src/web/ng/Server.cpp @@ -27,6 +27,7 @@ #include "web/ng/Connection.hpp" #include "web/ng/MessageHandler.hpp" #include "web/ng/ProcessingPolicy.hpp" +#include "web/ng/Response.hpp" #include "web/ng/impl/HttpConnection.hpp" #include "web/ng/impl/ServerSslContext.hpp" @@ -42,6 +43,7 @@ #include #include #include +#include #include #include @@ -120,7 +122,7 @@ detectSsl(boost::asio::ip::tcp::socket socket, boost::asio::yield_context yield) return SslDetectionResult{.socket = tcpStream.release_socket(), .isSsl = isSsl, .buffer = std::move(buffer)}; } -std::expected> +std::expected> makeConnection( SslDetectionResult sslDetectionResult, std::optional& sslContext, @@ -133,7 +135,7 @@ makeConnection( impl::UpgradableConnectionPtr connection; if (sslDetectionResult.isSsl) { if (not sslContext.has_value()) - return std::unexpected{"SSL is not supported by this server"}; + return std::unexpected{"Error creating a connection: SSL is not supported by this server"}; connection = std::make_unique( std::move(sslDetectionResult.socket), @@ -157,7 +159,17 @@ makeConnection( connection->close(yield); return std::unexpected{std::nullopt}; } + return connection; +} +std::expected +tryUpgradeConnection( + impl::UpgradableConnectionPtr connection, + std::optional& sslContext, + util::TagDecoratorFactory& tagDecoratorFactory, + boost::asio::yield_context yield +) +{ auto const expectedIsUpgrade = connection->isUpgradeRequested(yield); if (not expectedIsUpgrade.has_value()) { return std::unexpected{ @@ -256,8 +268,9 @@ Server::run() } void -Server::stop() +Server::stop(boost::asio::yield_context yield) { + connectionHandler_.stop(yield); } void @@ -288,15 +301,32 @@ Server::handleConnection(boost::asio::ip::tcp::socket socket, boost::asio::yield ); if (not connectionExpected.has_value()) { if (connectionExpected.error().has_value()) { - LOG(log_.info()) << "Error creating a connection: " << *connectionExpected.error(); + LOG(log_.info()) << *connectionExpected.error(); } return; } LOG(log_.trace()) << connectionExpected.value()->tag() << "Connection created"; + if (connectionHandler_.isStopping()) { + boost::asio::spawn( + ctx_.get(), + [connection = std::move(connectionExpected).value()](boost::asio::yield_context yield) { + web::ng::impl::ConnectionHandler::stopConnection(*connection, yield); + } + ); + return; + } + + auto connection = + tryUpgradeConnection(std::move(connectionExpected).value(), sslContext_, tagDecoratorFactory_, yield); + if (not connection.has_value()) { + LOG(log_.info()) << connection.error(); + return; + } + boost::asio::spawn( ctx_.get(), - [this, connection = std::move(connectionExpected).value()](boost::asio::yield_context yield) mutable { + [this, connection = std::move(connection).value()](boost::asio::yield_context yield) mutable { connectionHandler_.processConnection(std::move(connection), yield); } ); diff --git a/src/web/ng/Server.hpp b/src/web/ng/Server.hpp index 220ebd293..f719b8dd2 100644 --- a/src/web/ng/Server.hpp +++ b/src/web/ng/Server.hpp @@ -33,6 +33,7 @@ #include #include +#include #include #include #include @@ -40,10 +41,20 @@ namespace web::ng { +/** + * @brief A tag class for server to help identify Server in templated code. + */ +struct ServerTag { + virtual ~ServerTag() = default; +}; + +template +concept SomeServer = std::derived_from; + /** * @brief Web server class. */ -class Server { +class Server : public ServerTag { public: /** * @brief Check to perform for each new client connection. The check takes client ip as input and returns a Response @@ -147,11 +158,13 @@ public: run(); /** - * @brief Stop the server. - ** @note Stopping the server cause graceful shutdown of all connections. And rejecting new connections. + * @brief Stop the server. This method will asynchronously sleep unless all the users are disconnected. + * @note Stopping the server cause graceful shutdown of all connections. And rejecting new connections. + * + * @param yield The coroutine context. */ void - stop(); + stop(boost::asio::yield_context yield); private: void diff --git a/src/web/ng/impl/ConnectionHandler.cpp b/src/web/ng/impl/ConnectionHandler.cpp index 2a672ba3f..27e8bc655 100644 --- a/src/web/ng/impl/ConnectionHandler.cpp +++ b/src/web/ng/impl/ConnectionHandler.cpp @@ -35,10 +35,13 @@ #include #include #include +#include +#include #include #include #include +#include #include #include #include @@ -138,8 +141,23 @@ ConnectionHandler::onWs(MessageHandler handler) void ConnectionHandler::processConnection(ConnectionPtr connectionPtr, boost::asio::yield_context yield) { + LOG(log_.trace()) << connectionPtr->tag() << "New connection"; auto& connectionRef = *connectionPtr; - auto signalConnection = onStop_.connect([&connectionRef, yield]() { connectionRef.close(yield); }); + + if (isStopping()) { + stopConnection(connectionRef, yield); + return; + } + ++connectionsCounter_.get(); + + // Using coroutine group here to wait for stopConnection() to finish before exiting this function and destroying + // connection. + util::CoroutineGroup stopTask{yield, 1}; + auto stopSignalConnection = onStop_.connect([&connectionRef, &stopTask, yield]() { + stopTask.spawn(yield, [&connectionRef](boost::asio::yield_context innerYield) { + stopConnection(connectionRef, innerYield); + }); + }); bool shouldCloseGracefully = false; @@ -173,21 +191,57 @@ ConnectionHandler::processConnection(ConnectionPtr connectionPtr, boost::asio::y } if (shouldCloseGracefully) { + connectionRef.setTimeout(kCLOSE_CONNECTION_TIMEOUT); connectionRef.close(yield); LOG(log_.trace()) << connectionRef.tag() << "Closed gracefully"; } - signalConnection.disconnect(); + stopSignalConnection.disconnect(); LOG(log_.trace()) << connectionRef.tag() << "Signal disconnected"; onDisconnectHook_(connectionRef); LOG(log_.trace()) << connectionRef.tag() << "Processing finished"; + + // Wait for a stopConnection() to finish if there is any to not have dangling reference in stopConnection(). + stopTask.asyncWait(yield); + + --connectionsCounter_.get(); + if (connectionsCounter_.get().value() == 0 && stopping_) + stopHelper_.readyToStop(); } void -ConnectionHandler::stop() +ConnectionHandler::stopConnection(Connection& connection, boost::asio::yield_context yield) { + util::Logger log{"WebServer"}; + LOG(log.trace()) << connection.tag() << "Stopping connection"; + Response response{ + boost::beast::http::status::service_unavailable, + "This Clio node is shutting down. Please try another node.", + connection + }; + connection.send(std::move(response), yield); + connection.setTimeout(kCLOSE_CONNECTION_TIMEOUT); + connection.close(yield); + LOG(log.trace()) << connection.tag() << "Connection closed"; +} + +void +ConnectionHandler::stop(boost::asio::yield_context yield) +{ + *stopping_ = true; onStop_(); + if (connectionsCounter_.get().value() == 0) + return; + + // Wait for server to disconnect all the users + stopHelper_.asyncWaitForStop(yield); +} + +bool +ConnectionHandler::isStopping() const +{ + return *stopping_; } bool @@ -211,7 +265,7 @@ ConnectionHandler::handleError(Error const& error, Connection const& connection) // Therefore, if we see a short read here, it has occurred // after the message has been completed, so it is safe to ignore it. if (error == boost::beast::http::error::end_of_stream || error == boost::asio::ssl::error::stream_truncated || - error == boost::asio::error::eof) + error == boost::asio::error::eof || error == boost::beast::error::timeout) return false; // WebSocket connection was gracefully closed @@ -308,7 +362,10 @@ ConnectionHandler::parallelRequestResponseLoop( ); } } + LOG(log_.trace()) << connection.tag() + << "Waiting processing tasks to finish. Number of tasks: " << tasksGroup.size(); tasksGroup.asyncWait(yield); + LOG(log_.trace()) << connection.tag() << "Processing is done"; return closeConnectionGracefully; } diff --git a/src/web/ng/impl/ConnectionHandler.hpp b/src/web/ng/impl/ConnectionHandler.hpp index 0572539a7..b3f17416a 100644 --- a/src/web/ng/impl/ConnectionHandler.hpp +++ b/src/web/ng/impl/ConnectionHandler.hpp @@ -19,8 +19,12 @@ #pragma once +#include "util/StopHelper.hpp" #include "util/Taggable.hpp" #include "util/log/Logger.hpp" +#include "util/prometheus/Gauge.hpp" +#include "util/prometheus/Label.hpp" +#include "util/prometheus/Prometheus.hpp" #include "web/SubscriptionContextInterface.hpp" #include "web/ng/Connection.hpp" #include "web/ng/Error.hpp" @@ -33,8 +37,11 @@ #include #include +#include +#include #include #include +#include #include #include #include @@ -77,6 +84,12 @@ private: std::optional wsHandler_; boost::signals2::signal onStop_; + std::unique_ptr stopping_ = std::make_unique(false); + + std::reference_wrapper connectionsCounter_ = + PrometheusService::gaugeInt("connections_total_number", util::prometheus::Labels{{{"status", "connected"}}}); + + util::StopHelper stopHelper_; public: ConnectionHandler( @@ -87,6 +100,8 @@ public: OnDisconnectHook onDisconnectHook ); + static constexpr std::chrono::milliseconds kCLOSE_CONNECTION_TIMEOUT{500}; + void onGet(std::string const& target, MessageHandler handler); @@ -99,8 +114,14 @@ public: void processConnection(ConnectionPtr connection, boost::asio::yield_context yield); + static void + stopConnection(Connection& connection, boost::asio::yield_context yield); + void - stop(); + stop(boost::asio::yield_context yield); + + bool + isStopping() const; private: /** diff --git a/src/web/ng/impl/HttpConnection.hpp b/src/web/ng/impl/HttpConnection.hpp index 6773ed200..b7bdfae6a 100644 --- a/src/web/ng/impl/HttpConnection.hpp +++ b/src/web/ng/impl/HttpConnection.hpp @@ -77,6 +77,7 @@ class HttpConnection : public UpgradableConnection { StreamType stream_; std::optional> request_; std::chrono::steady_clock::duration timeout_{kDEFAULT_TIMEOUT}; + bool closed_{false}; public: HttpConnection( @@ -152,6 +153,13 @@ public: void close(boost::asio::yield_context yield) override { + // This is needed because calling async_shutdown() multiple times may lead to hanging coroutines. + // See WsConnection for more details. + if (closed_) + return; + + closed_ = true; + [[maybe_unused]] boost::system::error_code error; if constexpr (IsSslTcpStream) { boost::beast::get_lowest_layer(stream_).expires_after(timeout_); diff --git a/src/web/ng/impl/WsConnection.hpp b/src/web/ng/impl/WsConnection.hpp index b921c037d..e13e51b66 100644 --- a/src/web/ng/impl/WsConnection.hpp +++ b/src/web/ng/impl/WsConnection.hpp @@ -64,6 +64,7 @@ template class WsConnection : public WsConnectionBase { boost::beast::websocket::stream stream_; boost::beast::http::request initialRequest_; + bool closed_{false}; public: WsConnection( @@ -159,6 +160,13 @@ public: void close(boost::asio::yield_context yield) override { + if (closed_) + return; + + // This should be set before the async_close(). Otherwise there is a possibility to have multiple coroutines + // waiting on async_close(), but only one will be woken up after the actual close happened, others will hang. + closed_ = true; + boost::system::error_code error; // unused stream_.async_close(boost::beast::websocket::close_code::normal, yield[error]); } diff --git a/tests/common/util/AsioContextTestFixture.hpp b/tests/common/util/AsioContextTestFixture.hpp index 715a53ddf..274491009 100644 --- a/tests/common/util/AsioContextTestFixture.hpp +++ b/tests/common/util/AsioContextTestFixture.hpp @@ -21,10 +21,14 @@ #include "util/LoggerFixtures.hpp" +#include #include #include +#include #include +#include #include +#include #include #include @@ -94,6 +98,38 @@ struct SyncAsioContextTest : virtual public NoLoggerFixture { runContext(); } + template + void + runSpawnWithTimeout(std::chrono::steady_clock::duration timeout, F&& f, bool allowMockLeak = false) + { + using namespace boost::asio; + + boost::asio::io_context timerCtx; + steady_timer timer{timerCtx, timeout}; + spawn(timerCtx, [this, &timer](yield_context yield) { + boost::system::error_code errorCode; + timer.async_wait(yield[errorCode]); + ctx_.stop(); + EXPECT_TRUE(false) << "Test timed out"; + }); + std::thread timerThread{[&timerCtx]() { timerCtx.run(); }}; + + testing::MockFunction call; + if (allowMockLeak) + testing::Mock::AllowLeak(&call); + + spawn(ctx_, [&](yield_context yield) { + f(yield); + call.Call(); + }); + + EXPECT_CALL(call, Call()); + runContext(); + + timerCtx.stop(); + timerThread.join(); + } + void runContext() { @@ -108,6 +144,15 @@ struct SyncAsioContextTest : virtual public NoLoggerFixture { ctx_.reset(); } + template + static void + runSyncOperation(F&& f) + { + boost::asio::io_service ioc; + boost::asio::spawn(ioc, f); + ioc.run(); + } + protected: boost::asio::io_context ctx_; }; diff --git a/tests/common/util/MockBackend.hpp b/tests/common/util/MockBackend.hpp index 0e11251af..bc8a1f434 100644 --- a/tests/common/util/MockBackend.hpp +++ b/tests/common/util/MockBackend.hpp @@ -211,6 +211,8 @@ struct MockBackend : public BackendInterface { MOCK_METHOD(void, doWriteLedgerObject, (std::string&&, std::uint32_t const, std::string&&), (override)); + MOCK_METHOD(void, waitForWritesToFinish, (), (override)); + MOCK_METHOD(bool, doFinishWrites, (), (override)); MOCK_METHOD(void, writeMPTHolders, (std::vector const&), (override)); diff --git a/tests/common/util/MockSource.hpp b/tests/common/util/MockSource.hpp index 04adef923..58984d485 100644 --- a/tests/common/util/MockSource.hpp +++ b/tests/common/util/MockSource.hpp @@ -23,7 +23,6 @@ #include "etl/Source.hpp" #include "feed/SubscriptionManagerInterface.hpp" #include "rpc/Errors.hpp" -#include "util/newconfig/ConfigDefinition.hpp" #include "util/newconfig/ObjectView.hpp" #include @@ -49,6 +48,7 @@ struct MockSource : etl::SourceBase { MOCK_METHOD(void, run, (), (override)); + MOCK_METHOD(void, stop, (boost::asio::yield_context), (override)); MOCK_METHOD(bool, isConnected, (), (const, override)); MOCK_METHOD(void, setForwarding, (bool), (override)); MOCK_METHOD(boost::json::object, toJson, (), (const, override)); @@ -89,6 +89,12 @@ public: mock_->run(); } + void + stop(boost::asio::yield_context yield) override + { + mock_->stop(yield); + } + bool isConnected() const override { diff --git a/tests/common/util/MockSubscriptionManager.hpp b/tests/common/util/MockSubscriptionManager.hpp index 186fbdba5..f595e02f7 100644 --- a/tests/common/util/MockSubscriptionManager.hpp +++ b/tests/common/util/MockSubscriptionManager.hpp @@ -102,6 +102,8 @@ struct MockSubscriptionManager : feed::SubscriptionManagerInterface { MOCK_METHOD(void, unsubProposedTransactions, (feed::SubscriberSharedPtr const&), (override)); MOCK_METHOD(boost::json::object, report, (), (const, override)); + + MOCK_METHOD(void, stop, (), (override)); }; template