From a2a37a928a3caf3e0e475294c6ed3e4e3dcc29c0 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Wed, 26 May 2021 17:45:41 -0500 Subject: [PATCH] Redesign stoppable object pattern --- Builds/CMake/RippledCore.cmake | 2 - Builds/CMake/deps/cassandra.cmake | 2 +- src/ripple/app/ledger/InboundLedgers.h | 4 +- src/ripple/app/ledger/InboundTransactions.h | 5 +- src/ripple/app/ledger/LedgerCleaner.h | 24 +- src/ripple/app/ledger/LedgerMaster.h | 14 +- src/ripple/app/ledger/LedgerReplayer.h | 8 +- src/ripple/app/ledger/OrderBookDB.cpp | 9 +- src/ripple/app/ledger/OrderBookDB.h | 4 +- src/ripple/app/ledger/impl/InboundLedgers.cpp | 22 +- .../app/ledger/impl/InboundTransactions.cpp | 22 +- src/ripple/app/ledger/impl/LedgerCleaner.cpp | 72 +-- src/ripple/app/ledger/impl/LedgerMaster.cpp | 22 +- src/ripple/app/ledger/impl/LedgerReplayer.cpp | 13 +- src/ripple/app/main/Application.cpp | 215 ++++--- src/ripple/app/main/Application.h | 9 +- src/ripple/app/main/CollectorManager.cpp | 4 +- src/ripple/app/main/CollectorManager.h | 8 +- src/ripple/app/main/GRPCServer.cpp | 6 +- src/ripple/app/main/GRPCServer.h | 12 +- src/ripple/app/main/LoadManager.cpp | 61 +- src/ripple/app/main/LoadManager.h | 25 +- src/ripple/app/main/Main.cpp | 2 +- src/ripple/app/main/NodeStoreScheduler.cpp | 48 +- src/ripple/app/main/NodeStoreScheduler.h | 24 +- src/ripple/app/misc/NetworkOPs.cpp | 25 +- src/ripple/app/misc/NetworkOPs.h | 8 +- src/ripple/app/misc/SHAMapStore.h | 10 +- src/ripple/app/misc/SHAMapStoreImp.cpp | 35 +- src/ripple/app/misc/SHAMapStoreImp.h | 22 +- .../backend/RelationalDBInterfacePostgres.cpp | 13 +- .../backend/RelationalDBInterfacePostgres.h | 11 + src/ripple/app/reporting/ReportingETL.cpp | 5 +- src/ripple/app/reporting/ReportingETL.h | 13 +- src/ripple/basics/PerfLog.h | 19 +- src/ripple/basics/impl/PerfLogImp.cpp | 22 +- src/ripple/basics/impl/PerfLogImp.h | 14 +- src/ripple/basics/impl/ResolverAsio.cpp | 1 - src/ripple/core/ClosureCounter.h | 56 +- src/ripple/core/Job.h | 3 + src/ripple/core/JobQueue.h | 38 +- src/ripple/core/Pg.cpp | 10 +- src/ripple/core/Pg.h | 13 +- src/ripple/core/Stoppable.h | 447 --------------- src/ripple/core/impl/JobQueue.cpp | 78 +-- src/ripple/core/impl/Stoppable.cpp | 221 ------- src/ripple/core/impl/Workers.cpp | 4 +- src/ripple/core/impl/Workers.h | 33 +- src/ripple/net/HTTPDownloader.h | 2 +- src/ripple/net/InfoSub.h | 8 +- src/ripple/net/RPCSub.h | 1 - src/ripple/net/ShardDownloader.md | 12 +- src/ripple/net/impl/HTTPDownloader.cpp | 7 +- src/ripple/net/impl/InfoSub.cpp | 9 - src/ripple/nodestore/Database.h | 25 +- src/ripple/nodestore/DatabaseRotating.h | 4 +- src/ripple/nodestore/DatabaseShard.h | 7 +- src/ripple/nodestore/Manager.h | 2 - src/ripple/nodestore/impl/Database.cpp | 43 +- src/ripple/nodestore/impl/DatabaseNodeImp.h | 13 +- .../nodestore/impl/DatabaseRotatingImp.cpp | 5 +- .../nodestore/impl/DatabaseRotatingImp.h | 7 +- .../nodestore/impl/DatabaseShardImp.cpp | 55 +- src/ripple/nodestore/impl/DatabaseShardImp.h | 15 +- src/ripple/nodestore/impl/ManagerImp.cpp | 10 +- src/ripple/nodestore/impl/ManagerImp.h | 2 - src/ripple/nodestore/impl/TaskQueue.cpp | 16 +- src/ripple/nodestore/impl/TaskQueue.h | 7 +- src/ripple/overlay/Overlay.h | 19 +- src/ripple/overlay/impl/OverlayImpl.cpp | 80 +-- src/ripple/overlay/impl/OverlayImpl.h | 33 +- src/ripple/overlay/make_Overlay.h | 2 - src/ripple/peerfinder/PeerfinderManager.h | 13 +- .../peerfinder/impl/PeerfinderManager.cpp | 29 +- src/ripple/peerfinder/make_Manager.h | 1 - src/ripple/rpc/ServerHandler.h | 2 - src/ripple/rpc/ShardArchiveHandler.h | 17 +- .../rpc/handlers/LedgerCleanerHandler.cpp | 4 +- src/ripple/rpc/impl/ServerHandlerImp.cpp | 31 +- src/ripple/rpc/impl/ServerHandlerImp.h | 14 +- src/ripple/rpc/impl/ShardArchiveHandler.cpp | 35 +- src/test/app/LedgerReplay_test.cpp | 18 +- src/test/basics/PerfLog_test.cpp | 315 ++++------ src/test/core/JobQueue_test.cpp | 12 +- src/test/core/Stoppable_test.cpp | 539 ------------------ src/test/core/Workers_test.cpp | 2 +- src/test/jtx/impl/Env.cpp | 2 +- src/test/net/DatabaseDownloader_test.cpp | 2 +- src/test/nodestore/DatabaseShard_test.cpp | 3 +- src/test/nodestore/Database_test.cpp | 74 +-- src/test/rpc/Subscribe_test.cpp | 2 +- src/test/shamap/common.h | 4 +- 92 files changed, 781 insertions(+), 2460 deletions(-) delete mode 100644 src/ripple/core/Stoppable.h delete mode 100644 src/ripple/core/impl/Stoppable.cpp delete mode 100644 src/test/core/Stoppable_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index e50b20cd6c..1b51d62292 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -459,7 +459,6 @@ target_sources (rippled PRIVATE src/ripple/core/impl/LoadMonitor.cpp src/ripple/core/impl/SNTPClock.cpp src/ripple/core/impl/SociDB.cpp - src/ripple/core/impl/Stoppable.cpp src/ripple/core/impl/TimeKeeper.cpp src/ripple/core/impl/Workers.cpp src/ripple/core/Pg.cpp @@ -765,7 +764,6 @@ target_sources (rippled PRIVATE src/test/core/CryptoPRNG_test.cpp src/test/core/JobQueue_test.cpp src/test/core/SociDB_test.cpp - src/test/core/Stoppable_test.cpp src/test/core/Workers_test.cpp #[===============================[ test sources: diff --git a/Builds/CMake/deps/cassandra.cmake b/Builds/CMake/deps/cassandra.cmake index 46142c8f09..ca19bd528f 100644 --- a/Builds/CMake/deps/cassandra.cmake +++ b/Builds/CMake/deps/cassandra.cmake @@ -47,7 +47,7 @@ if(reporting) GIT_REPOSITORY https://github.com/krb5/krb5.git GIT_TAG master UPDATE_COMMAND "" - CONFIGURE_COMMAND autoreconf src && ./src/configure --enable-static --disable-shared > /dev/null + CONFIGURE_COMMAND autoreconf src && CFLAGS=-fcommon ./src/configure --enable-static --disable-shared > /dev/null BUILD_IN_SOURCE 1 BUILD_COMMAND make INSTALL_COMMAND "" diff --git a/src/ripple/app/ledger/InboundLedgers.h b/src/ripple/app/ledger/InboundLedgers.h index ae27b03d9c..dca4a80c54 100644 --- a/src/ripple/app/ledger/InboundLedgers.h +++ b/src/ripple/app/ledger/InboundLedgers.h @@ -21,7 +21,6 @@ #define RIPPLE_APP_LEDGER_INBOUNDLEDGERS_H_INCLUDED #include -#include #include #include @@ -83,14 +82,13 @@ public: sweep() = 0; virtual void - onStop() = 0; + stop() = 0; }; std::unique_ptr make_InboundLedgers( Application& app, InboundLedgers::clock_type& clock, - Stoppable& parent, beast::insight::Collector::ptr const& collector); } // namespace ripple diff --git a/src/ripple/app/ledger/InboundTransactions.h b/src/ripple/app/ledger/InboundTransactions.h index 1275ca77b1..f5b44f3861 100644 --- a/src/ripple/app/ledger/InboundTransactions.h +++ b/src/ripple/app/ledger/InboundTransactions.h @@ -21,7 +21,6 @@ #define RIPPLE_APP_LEDGER_INBOUNDTRANSACTIONS_H_INCLUDED #include -#include #include #include #include @@ -85,12 +84,14 @@ public: */ virtual void newRound(std::uint32_t seq) = 0; + + virtual void + stop() = 0; }; std::unique_ptr make_InboundTransactions( Application& app, - Stoppable& parent, beast::insight::Collector::ptr const& collector, std::function const&, bool)> gotSet); diff --git a/src/ripple/app/ledger/LedgerCleaner.h b/src/ripple/app/ledger/LedgerCleaner.h index d3ee4add06..9f82a851f2 100644 --- a/src/ripple/app/ledger/LedgerCleaner.h +++ b/src/ripple/app/ledger/LedgerCleaner.h @@ -23,27 +23,32 @@ #include #include #include -#include #include #include namespace ripple { -namespace detail { /** Check the ledger/transaction databases to make sure they have continuity */ -class LedgerCleaner : public Stoppable, public beast::PropertyStream::Source +class LedgerCleaner : public beast::PropertyStream::Source { protected: - explicit LedgerCleaner(Stoppable& parent); + LedgerCleaner() : beast::PropertyStream::Source("ledgercleaner") + { + } public: - /** Destroy the object. */ - virtual ~LedgerCleaner() = 0; + virtual ~LedgerCleaner() = default; + + virtual void + start() = 0; + + virtual void + stop() = 0; /** Start a long running task to clean the ledger. The ledger is cleaned asynchronously, on an implementation defined thread. This function call does not block. The long running task - will be stopped if the Stoppable stops. + will be stopped by a call to stop(). Thread safety: Safe to call from any thread at any time. @@ -51,13 +56,12 @@ public: @param parameters A Json object with configurable parameters. */ virtual void - doClean(Json::Value const& parameters) = 0; + clean(Json::Value const& parameters) = 0; }; std::unique_ptr -make_LedgerCleaner(Application& app, Stoppable& parent, beast::Journal journal); +make_LedgerCleaner(Application& app, beast::Journal journal); -} // namespace detail } // namespace ripple #endif diff --git a/src/ripple/app/ledger/LedgerMaster.h b/src/ripple/app/ledger/LedgerMaster.h index 1a6cafa42d..e886c4651f 100644 --- a/src/ripple/app/ledger/LedgerMaster.h +++ b/src/ripple/app/ledger/LedgerMaster.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -31,10 +30,9 @@ #include #include #include +#include #include #include -#include -#include #include #include #include @@ -69,7 +67,7 @@ public: // Tracks the current ledger and any ledgers in the process of closing // Tracks ledger history // Tracks held transactions -class LedgerMaster : public Stoppable, public AbstractFetchPackContainer +class LedgerMaster : public AbstractFetchPackContainer { public: // Age for last validated ledger if the process has yet to validate. @@ -79,7 +77,6 @@ public: explicit LedgerMaster( Application& app, Stopwatch& stopwatch, - Stoppable& parent, beast::insight::Collector::ptr const& collector, beast::Journal journal); @@ -259,11 +256,6 @@ public: bool fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash); - void - doLedgerCleaner(Json::Value const& parameters); - - beast::PropertyStream::Source& - getPropertySource(); void clearPriorLedgers(LedgerIndex seq); @@ -385,8 +377,6 @@ private: std::recursive_mutex mCompleteLock; RangeSet mCompleteLedgers; - std::unique_ptr mLedgerCleaner; - // Publish thread is running. bool mAdvanceThread{false}; diff --git a/src/ripple/app/ledger/LedgerReplayer.h b/src/ripple/app/ledger/LedgerReplayer.h index 5ef9906de4..e9e94548b7 100644 --- a/src/ripple/app/ledger/LedgerReplayer.h +++ b/src/ripple/app/ledger/LedgerReplayer.h @@ -24,7 +24,6 @@ #include #include #include -#include #include #include @@ -70,14 +69,13 @@ std::uint32_t constexpr MAX_QUEUED_TASKS = 100; /** * Manages the lifetime of ledger replay tasks. */ -class LedgerReplayer final : public Stoppable +class LedgerReplayer final { public: LedgerReplayer( Application& app, InboundLedgers& inboundLedgers, - std::unique_ptr peerSetBuilder, - Stoppable& parent); + std::unique_ptr peerSetBuilder); ~LedgerReplayer(); @@ -125,7 +123,7 @@ public: sweep(); void - onStop() override; + stop(); private: mutable std::mutex mtx_; diff --git a/src/ripple/app/ledger/OrderBookDB.cpp b/src/ripple/app/ledger/OrderBookDB.cpp index 98cb306d5b..a81331d04e 100644 --- a/src/ripple/app/ledger/OrderBookDB.cpp +++ b/src/ripple/app/ledger/OrderBookDB.cpp @@ -27,11 +27,8 @@ namespace ripple { -OrderBookDB::OrderBookDB(Application& app, Stoppable& parent) - : Stoppable("OrderBookDB", parent) - , app_(app) - , mSeq(0) - , j_(app.journal("OrderBookDB")) +OrderBookDB::OrderBookDB(Application& app) + : app_(app), mSeq(0), j_(app.journal("OrderBookDB")) { } @@ -101,7 +98,7 @@ OrderBookDB::update(std::shared_ptr const& ledger) { for (auto& sle : ledger->sles) { - if (isStopping()) + if (app_.isStopping()) { JLOG(j_.info()) << "OrderBookDB::update exiting due to isStopping"; diff --git a/src/ripple/app/ledger/OrderBookDB.h b/src/ripple/app/ledger/OrderBookDB.h index 4a79bc1293..a1983fdb40 100644 --- a/src/ripple/app/ledger/OrderBookDB.h +++ b/src/ripple/app/ledger/OrderBookDB.h @@ -28,10 +28,10 @@ namespace ripple { -class OrderBookDB : public Stoppable +class OrderBookDB { public: - OrderBookDB(Application& app, Stoppable& parent); + OrderBookDB(Application& app); void setup(std::shared_ptr const& ledger); diff --git a/src/ripple/app/ledger/impl/InboundLedgers.cpp b/src/ripple/app/ledger/impl/InboundLedgers.cpp index 9d04ff1eee..2f8a07138d 100644 --- a/src/ripple/app/ledger/impl/InboundLedgers.cpp +++ b/src/ripple/app/ledger/impl/InboundLedgers.cpp @@ -33,7 +33,7 @@ namespace ripple { -class InboundLedgersImp : public InboundLedgers, public Stoppable +class InboundLedgersImp : public InboundLedgers { private: Application& app_; @@ -49,11 +49,9 @@ public: InboundLedgersImp( Application& app, clock_type& clock, - Stoppable& parent, beast::insight::Collector::ptr const& collector, std::unique_ptr peerSetBuilder) - : Stoppable("InboundLedgers", parent) - , app_(app) + : app_(app) , fetchRate_(clock.now()) , j_(app.journal("InboundLedger")) , m_clock(clock) @@ -74,13 +72,15 @@ public: assert( reason != InboundLedger::Reason::SHARD || (seq != 0 && app_.getShardStore())); - if (isStopping()) - return {}; bool isNew = true; std::shared_ptr inbound; { ScopedLockType sl(mLock); + if (stopping_) + { + return {}; + } auto it = mLedgers.find(hash); if (it != mLedgers.end()) { @@ -388,14 +388,12 @@ public: } void - onStop() override + stop() override { ScopedLockType lock(mLock); - + stopping_ = true; mLedgers.clear(); mRecentFailures.clear(); - - stopped(); } private: @@ -404,6 +402,7 @@ private: using ScopedLockType = std::unique_lock; std::recursive_mutex mLock; + bool stopping_ = false; using MapType = hash_map>; MapType mLedgers; @@ -420,11 +419,10 @@ std::unique_ptr make_InboundLedgers( Application& app, InboundLedgers::clock_type& clock, - Stoppable& parent, beast::insight::Collector::ptr const& collector) { return std::make_unique( - app, clock, parent, collector, make_PeerSetBuilder(app)); + app, clock, collector, make_PeerSetBuilder(app)); } } // namespace ripple diff --git a/src/ripple/app/ledger/impl/InboundTransactions.cpp b/src/ripple/app/ledger/impl/InboundTransactions.cpp index 662f8392ec..7bccf26aa4 100644 --- a/src/ripple/app/ledger/impl/InboundTransactions.cpp +++ b/src/ripple/app/ledger/impl/InboundTransactions.cpp @@ -58,19 +58,15 @@ public: } }; -class InboundTransactionsImp : public InboundTransactions, public Stoppable +class InboundTransactionsImp : public InboundTransactions { public: - Application& app_; - InboundTransactionsImp( Application& app, - Stoppable& parent, beast::insight::Collector::ptr const& collector, std::function const&, bool)> gotSet, std::unique_ptr peerSetBuilder) - : Stoppable("InboundTransactions", parent) - , app_(app) + : app_(app) , m_seq(0) , m_zeroSet(m_map[uint256()]) , m_gotSet(std::move(gotSet)) @@ -118,7 +114,7 @@ public: return it->second.mSet; } - if (!acquire || isStopping()) + if (!acquire || stopping_) return std::shared_ptr(); ta = std::make_shared( @@ -242,20 +238,21 @@ public: } void - onStop() override + stop() override { std::lock_guard lock(mLock); - + stopping_ = true; m_map.clear(); - - stopped(); } private: using MapType = hash_map; + Application& app_; + std::recursive_mutex mLock; + bool stopping_{false}; MapType m_map; std::uint32_t m_seq; @@ -274,12 +271,11 @@ InboundTransactions::~InboundTransactions() = default; std::unique_ptr make_InboundTransactions( Application& app, - Stoppable& parent, beast::insight::Collector::ptr const& collector, std::function const&, bool)> gotSet) { return std::make_unique( - app, parent, collector, std::move(gotSet), make_PeerSetBuilder(app)); + app, collector, std::move(gotSet), make_PeerSetBuilder(app)); } } // namespace ripple diff --git a/src/ripple/app/ledger/impl/LedgerCleaner.cpp b/src/ripple/app/ledger/impl/LedgerCleaner.cpp index b9138b6988..e5ee6409d3 100644 --- a/src/ripple/app/ledger/impl/LedgerCleaner.cpp +++ b/src/ripple/app/ledger/impl/LedgerCleaner.cpp @@ -25,7 +25,6 @@ #include namespace ripple { -namespace detail { /* @@ -51,8 +50,8 @@ class LedgerCleanerImp : public LedgerCleaner std::thread thread_; - enum class State : char { readyToClean = 0, startCleaning, cleaning }; - State state_ = State::readyToClean; + enum class State : char { notCleaning = 0, cleaning }; + State state_ = State::notCleaning; bool shouldExit_ = false; // The lowest ledger in the range we're checking. @@ -72,34 +71,25 @@ class LedgerCleanerImp : public LedgerCleaner //-------------------------------------------------------------------------- public: - LedgerCleanerImp( - Application& app, - Stoppable& stoppable, - beast::Journal journal) - : LedgerCleaner(stoppable), app_(app), j_(journal) + LedgerCleanerImp(Application& app, beast::Journal journal) + : app_(app), j_(journal) { } ~LedgerCleanerImp() override { if (thread_.joinable()) - LogicError("LedgerCleanerImp::onStop not called."); + LogicError("LedgerCleanerImp::stop not called."); } - //-------------------------------------------------------------------------- - // - // Stoppable - // - //-------------------------------------------------------------------------- - void - onStart() override + start() override { thread_ = std::thread{&LedgerCleanerImp::run, this}; } void - onStop() override + stop() override { JLOG(j_.info()) << "Stopping"; { @@ -142,7 +132,7 @@ public: //-------------------------------------------------------------------------- void - doClean(Json::Value const& params) override + clean(Json::Value const& params) override { LedgerIndex minRange = 0; LedgerIndex maxRange = 0; @@ -214,11 +204,8 @@ public: if (params.isMember(jss::stop) && params[jss::stop].asBool()) minRange_ = maxRange_ = 0; - if (state_ == State::readyToClean) - { - state_ = State::startCleaning; - wakeup_.notify_one(); - } + state_ = State::cleaning; + wakeup_.notify_one(); } } @@ -228,36 +215,26 @@ public: // //-------------------------------------------------------------------------- private: - void - init() - { - JLOG(j_.debug()) << "Initializing"; - } - void run() { beast::setCurrentThreadName("LedgerCleaner"); JLOG(j_.debug()) << "Started"; - init(); - while (true) { { std::unique_lock lock(mutex_); + state_ = State::notCleaning; wakeup_.wait(lock, [this]() { - return (shouldExit_ || state_ == State::startCleaning); + return (shouldExit_ || state_ == State::cleaning); }); if (shouldExit_) break; - - state_ = State::cleaning; + assert(state_ == State::cleaning); } doLedgerCleaner(); } - - stopped(); } // VFALCO TODO This should return std::optional @@ -399,7 +376,7 @@ private: void doLedgerCleaner() { - auto shouldExit = [this]() { + auto shouldExit = [this] { std::lock_guard lock(mutex_); return shouldExit_; }; @@ -413,12 +390,11 @@ private: bool doNodes; bool doTxns; - while (app_.getFeeTrack().isLoadedLocal()) + if (app_.getFeeTrack().isLoadedLocal()) { JLOG(j_.debug()) << "Waiting for load to subside"; std::this_thread::sleep_for(std::chrono::seconds(5)); - if (shouldExit()) - return; + continue; } { @@ -427,7 +403,6 @@ private: (minRange_ == 0)) { minRange_ = maxRange_ = 0; - state_ = State::readyToClean; return; } ledgerIndex = maxRange_; @@ -476,21 +451,10 @@ private: } }; -//------------------------------------------------------------------------------ - -LedgerCleaner::LedgerCleaner(Stoppable& parent) - : Stoppable("LedgerCleaner", parent) - , beast::PropertyStream::Source("ledgercleaner") -{ -} - -LedgerCleaner::~LedgerCleaner() = default; - std::unique_ptr -make_LedgerCleaner(Application& app, Stoppable& parent, beast::Journal journal) +make_LedgerCleaner(Application& app, beast::Journal journal) { - return std::make_unique(app, parent, journal); + return std::make_unique(app, journal); } -} // namespace detail } // namespace ripple diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index 4ea804ba5c..2c9aa150df 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -184,15 +184,11 @@ shouldAcquire( LedgerMaster::LedgerMaster( Application& app, Stopwatch& stopwatch, - Stoppable& parent, beast::insight::Collector::ptr const& collector, beast::Journal journal) - : Stoppable("LedgerMaster", parent) - , app_(app) + : app_(app) , m_journal(journal) , mLedgerHistory(collector, app) - , mLedgerCleaner( - detail::make_LedgerCleaner(app, *this, app_.journal("LedgerCleaner"))) , standalone_(app_.config().standalone()) , fetch_depth_( app_.getSHAMapStore().clampFetchDepth(app_.config().FETCH_DEPTH)) @@ -747,7 +743,7 @@ LedgerMaster::tryFill(Job& job, std::shared_ptr ledger) if (it == ledgerHashes.end()) { - if (app_.isShutdown()) + if (app_.isStopping()) return; { @@ -1614,7 +1610,7 @@ LedgerMaster::newPFWork( } // If we're stopping don't give callers the expectation that their // request will be fulfilled, even if it may be serviced. - return mPathFindThread > 0 && !isStopping(); + return mPathFindThread > 0 && !app_.isStopping(); } std::recursive_mutex& @@ -1838,12 +1834,6 @@ LedgerMaster::getLedgerByHash(uint256 const& hash) return {}; } -void -LedgerMaster::doLedgerCleaner(Json::Value const& parameters) -{ - mLedgerCleaner->doClean(parameters); -} - void LedgerMaster::setLedgerRangePresent(std::uint32_t minV, std::uint32_t maxV) { @@ -1870,12 +1860,6 @@ LedgerMaster::getCacheHitRate() return mLedgerHistory.getCacheHitRate(); } -beast::PropertyStream::Source& -LedgerMaster::getPropertySource() -{ - return *mLedgerCleaner; -} - void LedgerMaster::clearPriorLedgers(LedgerIndex seq) { diff --git a/src/ripple/app/ledger/impl/LedgerReplayer.cpp b/src/ripple/app/ledger/impl/LedgerReplayer.cpp index b9d4d014e3..9ec5c6f2cb 100644 --- a/src/ripple/app/ledger/impl/LedgerReplayer.cpp +++ b/src/ripple/app/ledger/impl/LedgerReplayer.cpp @@ -27,10 +27,8 @@ namespace ripple { LedgerReplayer::LedgerReplayer( Application& app, InboundLedgers& inboundLedgers, - std::unique_ptr peerSetBuilder, - Stoppable& parent) - : Stoppable("LedgerReplayer", parent) - , app_(app) + std::unique_ptr peerSetBuilder) + : app_(app) , inboundLedgers_(inboundLedgers) , peerSetBuilder_(std::move(peerSetBuilder)) , j_(app.journal("LedgerReplayer")) @@ -61,7 +59,7 @@ LedgerReplayer::replay( bool newSkipList = false; { std::lock_guard lock(mtx_); - if (isStopping()) + if (app_.isStopping()) return; if (tasks_.size() >= LedgerReplayParameters::MAX_TASKS) { @@ -145,7 +143,7 @@ LedgerReplayer::createDeltas(std::shared_ptr task) bool newDelta = false; { std::lock_guard lock(mtx_); - if (isStopping()) + if (app_.isStopping()) return; auto i = deltas_.find(*skipListItem); if (i != deltas_.end()) @@ -256,7 +254,7 @@ LedgerReplayer::sweep() } void -LedgerReplayer::onStop() +LedgerReplayer::stop() { JLOG(j_.info()) << "Stopping..."; { @@ -276,7 +274,6 @@ LedgerReplayer::onStop() deltas_.clear(); } - stopped(); JLOG(j_.info()) << "Stopped"; } diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index ad349e2806..3f80c2ae01 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +56,6 @@ #include #include #include -#include #include #include #include @@ -91,7 +91,7 @@ namespace ripple { // VFALCO TODO Move the function definitions into the class declaration -class ApplicationImp : public Application, public RootStoppable, public BasicApp +class ApplicationImp : public Application, public BasicApp { private: class io_latency_sampler @@ -170,23 +170,21 @@ public: // Required by the SHAMapStore TransactionMaster m_txMaster; + std::unique_ptr m_collectorManager; + std::unique_ptr m_jobQueue; NodeStoreScheduler m_nodeStoreScheduler; std::unique_ptr m_shaMapStore; PendingSaves pendingSaves_; AccountIDCache accountIDCache_; std::optional openLedger_; - // These are not Stoppable-derived NodeCache m_tempNodeCache; - std::unique_ptr m_collectorManager; CachedSLEs cachedSLEs_; std::pair nodeIdentity_; ValidatorKeys const validatorKeys_; std::unique_ptr m_resourceManager; - // These are Stoppable-related - std::unique_ptr m_jobQueue; std::unique_ptr m_nodeStore; NodeFamily nodeFamily_; std::unique_ptr shardStore_; @@ -196,6 +194,7 @@ public: OrderBookDB m_orderBookDB; std::unique_ptr m_pathRequests; std::unique_ptr m_ledgerMaster; + std::unique_ptr ledgerCleaner_; std::unique_ptr m_inboundLedgers; std::unique_ptr m_inboundTransactions; std::unique_ptr m_ledgerReplayer; @@ -217,17 +216,15 @@ public: ClosureCounter waitHandlerCounter_; boost::asio::steady_timer sweepTimer_; boost::asio::steady_timer entropyTimer_; - bool startTimers_; std::unique_ptr mRelationalDBInterface; std::unique_ptr mWalletDB; std::unique_ptr overlay_; - std::vector> websocketServers_; boost::asio::signal_set m_signals; std::condition_variable cv_; - std::mutex mut_; + mutable std::mutex mut_; bool isTimeToStop = false; std::atomic checkSigs_; @@ -265,8 +262,7 @@ public: std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper) - : RootStoppable("Application") - , BasicApp(numberOfThreads(*config)) + : BasicApp(numberOfThreads(*config)) , config_(std::move(config)) , logs_(std::move(logs)) , timeKeeper_(std::move(timeKeeper)) @@ -277,15 +273,24 @@ public: perf::setup_PerfLog( config_->section("perf"), config_->CONFIG_DIR), - *this, logs_->journal("PerfLog"), - [this]() { signalStop(); })) + [this] { signalStop(); })) , m_txMaster(*this) - , m_nodeStoreScheduler(*this) + , m_collectorManager(make_CollectorManager( + config_->section(SECTION_INSIGHT), + logs_->journal("Collector"))) + + , m_jobQueue(std::make_unique( + m_collectorManager->group("jobq"), + logs_->journal("JobQueue"), + *logs_, + *perfLog_)) + + , m_nodeStoreScheduler(*m_jobQueue) + , m_shaMapStore(make_SHAMapStore( - *this, *this, m_nodeStoreScheduler, logs_->journal("SHAMapStore"))) @@ -299,9 +304,6 @@ public: stopwatch(), logs_->journal("TaggedCache")) - , m_collectorManager(CollectorManager::New( - config_->section(SECTION_INSIGHT), - logs_->journal("Collector"))) , cachedSLEs_(std::chrono::minutes(1), stopwatch()) , validatorKeys_(*config_, m_journal) @@ -309,29 +311,18 @@ public: m_collectorManager->collector(), logs_->journal("Resource"))) - // The JobQueue has to come pretty early since - // almost everything is a Stoppable child of the JobQueue. - // - , m_jobQueue(std::make_unique( - m_collectorManager->group("jobq"), - m_nodeStoreScheduler, - logs_->journal("JobQueue"), - *logs_, - *perfLog_)) - - , m_nodeStore(m_shaMapStore->makeNodeStore("NodeStore.main", 4)) + , m_nodeStore(m_shaMapStore->makeNodeStore(4)) , nodeFamily_(*this, *m_collectorManager) // The shard store is optional and make_ShardStore can return null. , shardStore_(make_ShardStore( *this, - *m_jobQueue, m_nodeStoreScheduler, 4, logs_->journal("ShardStore"))) - , m_orderBookDB(*this, *m_jobQueue) + , m_orderBookDB(*this) , m_pathRequests(std::make_unique( *this, @@ -341,22 +332,22 @@ public: , m_ledgerMaster(std::make_unique( *this, stopwatch(), - *m_jobQueue, m_collectorManager->collector(), logs_->journal("LedgerMaster"))) + , ledgerCleaner_( + make_LedgerCleaner(*this, logs_->journal("LedgerCleaner"))) + // VFALCO NOTE must come before NetworkOPs to prevent a crash due // to dependencies in the destructor. // , m_inboundLedgers(make_InboundLedgers( *this, stopwatch(), - *m_jobQueue, m_collectorManager->collector())) , m_inboundTransactions(make_InboundTransactions( *this, - *m_jobQueue, m_collectorManager->collector(), [this](std::shared_ptr const& set, bool fromAcquire) { gotTXSet(set, fromAcquire); @@ -365,8 +356,7 @@ public: , m_ledgerReplayer(std::make_unique( *this, *m_inboundLedgers, - make_PeerSetBuilder(*this), - *m_jobQueue)) + make_PeerSetBuilder(*this))) , m_acceptedLedgerCache( "AcceptedLedger", @@ -383,7 +373,6 @@ public: config_->START_VALID, *m_jobQueue, *m_ledgerMaster, - *m_jobQueue, validatorKeys_, get_io_service(), logs_->journal("NetworkOPs"), @@ -412,7 +401,6 @@ public: , serverHandler_(make_ServerHandler( *this, - *m_networkOPs, get_io_service(), *m_jobQueue, *m_networkOPs, @@ -433,8 +421,7 @@ public: *this, logs_->journal("Validations")) - , m_loadManager( - make_LoadManager(*this, *this, logs_->journal("LoadManager"))) + , m_loadManager(make_LoadManager(*this, logs_->journal("LoadManager"))) , txQ_( std::make_unique(setup_TxQ(*config_), logs_->journal("TxQ"))) @@ -443,8 +430,6 @@ public: , entropyTimer_(get_io_service()) - , startTimers_(false) - , m_signals(get_io_service()) , checkSigs_(true) @@ -457,8 +442,10 @@ public: logs_->journal("Application"), std::chrono::milliseconds(100), get_io_service()) - , grpcServer_(std::make_unique(*this, *m_jobQueue)) - , reportingETL_(std::make_unique(*this, *m_ledgerMaster)) + , grpcServer_(std::make_unique(*this)) + , reportingETL_( + config_->reporting() ? std::make_unique(*this) + : nullptr) { add(m_resourceManager.get()); @@ -466,8 +453,8 @@ public: // VFALCO - READ THIS! // // Do not start threads, open sockets, or do any sort of "real work" - // inside the constructor. Put it in onStart instead. Or if you must, - // put it in setup (but everything in setup should be moved to onStart + // inside the constructor. Put it in start instead. Or if you must, + // put it in setup (but everything in setup should be moved to start // anyway. // // The reason is that the unit tests require an Application object to @@ -477,10 +464,7 @@ public: // started in this constructor. // - // VFALCO HACK - m_nodeStoreScheduler.setJobQueue(*m_jobQueue); - - add(m_ledgerMaster->getPropertySource()); + add(ledgerCleaner_.get()); } //-------------------------------------------------------------------------- @@ -488,17 +472,17 @@ public: bool setup() override; void - doStart(bool withTimers) override; + start(bool withTimers) override; void run() override; - bool - isShutdown() override; void signalStop() override; bool checkSigs() const override; void checkSigs(bool) override; + bool + isStopping() const override; int fdRequired() const override; @@ -584,6 +568,12 @@ public: return *m_ledgerMaster; } + LedgerCleaner& + getLedgerCleaner() override + { + return *ledgerCleaner_; + } + LedgerReplayer& getLedgerReplayer() override { @@ -685,8 +675,8 @@ public: return nullptr; } - auto handler = RPC::ShardArchiveHandler::tryMakeRecoveryHandler( - *this, *m_jobQueue); + auto handler = + RPC::ShardArchiveHandler::tryMakeRecoveryHandler(*this); if (!initAndSet(std::move(handler))) return nullptr; @@ -695,8 +685,8 @@ public: // Construct the ShardArchiveHandler if (shardArchiveHandler_ == nullptr) { - auto handler = RPC::ShardArchiveHandler::makeShardArchiveHandler( - *this, *m_jobQueue); + auto handler = + RPC::ShardArchiveHandler::makeShardArchiveHandler(*this); if (!initAndSet(std::move(handler))) return nullptr; @@ -911,15 +901,12 @@ public: { auto j = logs_->journal("NodeObject"); NodeStore::DummyScheduler dummyScheduler; - RootStoppable dummyRoot{"DummyRoot"}; std::unique_ptr source = NodeStore::Manager::instance().make_Database( - "NodeStore.import", megabytes(config_->getValueFor( SizedItem::burstSize, std::nullopt)), dummyScheduler, 0, - dummyRoot, config_->section(ConfigSection::importNodeDatabase()), j); @@ -948,31 +935,10 @@ public: } //-------------------------------------------------------------------------- - // - // Stoppable - // - - void - onStart() override - { - JLOG(m_journal.info()) << "Application starting. Version is " - << BuildInfo::getVersionString(); - - using namespace std::chrono_literals; - if (startTimers_) - { - setSweepTimer(); - setEntropyTimer(); - } - - m_io_latency_sampler.start(); - - m_resolver->start(); - } // Called to indicate shutdown. void - onStop() override + stop() { JLOG(m_journal.debug()) << "Application stopping"; @@ -1035,7 +1001,31 @@ public: return validators().trustedPublisher(pubKey); }); - stopped(); + // The order of these stop calls is delicate. + // Re-ordering them risks undefined behavior. + m_loadManager->stop(); + m_shaMapStore->stop(); + m_jobQueue->stop(); + if (shardArchiveHandler_) + shardArchiveHandler_->stop(); + if (overlay_) + overlay_->stop(); + if (shardStore_) + shardStore_->stop(); + grpcServer_->stop(); + m_networkOPs->stop(); + serverHandler_->stop(); + m_ledgerReplayer->stop(); + m_inboundTransactions->stop(); + m_inboundLedgers->stop(); + ledgerCleaner_->stop(); + if (reportingETL_) + reportingETL_->stop(); + if (auto pg = dynamic_cast( + &*mRelationalDBInterface)) + pg->stop(); + m_nodeStore->stop(); + perfLog_->stop(); } //-------------------------------------------------------------------------- @@ -1056,8 +1046,7 @@ public: // Only start the timer if waitHandlerCounter_ is not yet joined. if (auto optionalCountedHandler = waitHandlerCounter_.wrap( [this](boost::system::error_code const& e) { - if ((e.value() == boost::system::errc::success) && - (!m_jobQueue->isStopped())) + if (e.value() == boost::system::errc::success) { m_jobQueue->addJob( jtSWEEP, "sweep", [this](Job&) { doSweep(); }); @@ -1139,10 +1128,9 @@ public: cachedSLEs_.expire(); #ifdef RIPPLED_REPORTING - if (config().reporting()) - dynamic_cast( - &*mRelationalDBInterface) - ->sweep(); + if (auto pg = dynamic_cast( + &*mRelationalDBInterface)) + pg->sweep(); #endif // Set timer to do another sweep later. @@ -1396,7 +1384,6 @@ ApplicationImp::setup() overlay_ = make_Overlay( *this, setup_Overlay(*config_), - *m_jobQueue, *serverHandler_, *m_resourceManager, *m_resolver, @@ -1561,19 +1548,33 @@ ApplicationImp::setup() validatorSites_->start(); - if (config_->reporting()) - { - reportingETL_->run(); - } + if (reportingETL_) + reportingETL_->start(); return true; } void -ApplicationImp::doStart(bool withTimers) +ApplicationImp::start(bool withTimers) { - startTimers_ = withTimers; - start(); + JLOG(m_journal.info()) << "Application starting. Version is " + << BuildInfo::getVersionString(); + + if (withTimers) + { + setSweepTimer(); + setEntropyTimer(); + } + + m_io_latency_sampler.start(); + m_resolver->start(); + m_loadManager->start(); + m_shaMapStore->start(); + if (overlay_) + overlay_->start(); + grpcServer_->start(); + ledgerCleaner_->start(); + perfLog_->start(); } void @@ -1593,10 +1594,8 @@ ApplicationImp::run() cv_.wait(lk, [this] { return isTimeToStop; }); } - // Stop the server. When this returns, all - // Stoppable objects should be stopped. JLOG(m_journal.info()) << "Received shutdown request"; - stop(m_journal); + stop(); JLOG(m_journal.info()) << "Done."; } @@ -1614,13 +1613,6 @@ ApplicationImp::signalStop() } } -bool -ApplicationImp::isShutdown() -{ - // from Stoppable mixin - return isStopped(); -} - bool ApplicationImp::checkSigs() const { @@ -1633,6 +1625,13 @@ ApplicationImp::checkSigs(bool check) checkSigs_ = check; } +bool +ApplicationImp::isStopping() const +{ + std::lock_guard lk{mut_}; + return isTimeToStop; +} + int ApplicationImp::fdRequired() const { @@ -2045,7 +2044,7 @@ ApplicationImp::serverOkay(std::string& reason) if (!config().ELB_SUPPORT) return true; - if (isShutdown()) + if (isStopping()) { reason = "Server is shutting down"; return false; diff --git a/src/ripple/app/main/Application.h b/src/ripple/app/main/Application.h index f0d00b0815..3c31e10bb9 100644 --- a/src/ripple/app/main/Application.h +++ b/src/ripple/app/main/Application.h @@ -64,6 +64,7 @@ class InboundTransactions; class AcceptedLedger; class Ledger; class LedgerMaster; +class LedgerCleaner; class LedgerReplayer; class LoadManager; class ManifestCache; @@ -125,17 +126,17 @@ public: virtual bool setup() = 0; virtual void - doStart(bool withTimers) = 0; + start(bool withTimers) = 0; virtual void run() = 0; - virtual bool - isShutdown() = 0; virtual void signalStop() = 0; virtual bool checkSigs() const = 0; virtual void checkSigs(bool) = 0; + virtual bool + isStopping() const = 0; // // --- @@ -205,6 +206,8 @@ public: virtual LedgerMaster& getLedgerMaster() = 0; + virtual LedgerCleaner& + getLedgerCleaner() = 0; virtual LedgerReplayer& getLedgerReplayer() = 0; virtual NetworkOPs& diff --git a/src/ripple/app/main/CollectorManager.cpp b/src/ripple/app/main/CollectorManager.cpp index d23aeb8a3f..0781b271e6 100644 --- a/src/ripple/app/main/CollectorManager.cpp +++ b/src/ripple/app/main/CollectorManager.cpp @@ -68,10 +68,8 @@ public: //------------------------------------------------------------------------------ -CollectorManager::~CollectorManager() = default; - std::unique_ptr -CollectorManager::New(Section const& params, beast::Journal journal) +make_CollectorManager(Section const& params, beast::Journal journal) { return std::make_unique(params, journal); } diff --git a/src/ripple/app/main/CollectorManager.h b/src/ripple/app/main/CollectorManager.h index 0b352a57a3..46e113082d 100644 --- a/src/ripple/app/main/CollectorManager.h +++ b/src/ripple/app/main/CollectorManager.h @@ -29,16 +29,18 @@ namespace ripple { class CollectorManager { public: - static std::unique_ptr - New(Section const& params, beast::Journal journal); + virtual ~CollectorManager() = default; - virtual ~CollectorManager() = 0; virtual beast::insight::Collector::ptr const& collector() = 0; + virtual beast::insight::Group::ptr const& group(std::string const& name) = 0; }; +std::unique_ptr +make_CollectorManager(Section const& params, beast::Journal journal); + } // namespace ripple #endif diff --git a/src/ripple/app/main/GRPCServer.cpp b/src/ripple/app/main/GRPCServer.cpp index 14792cd90d..efe7474742 100644 --- a/src/ripple/app/main/GRPCServer.cpp +++ b/src/ripple/app/main/GRPCServer.cpp @@ -774,7 +774,7 @@ GRPCServerImpl::start() } void -GRPCServer::onStart() +GRPCServer::start() { // Start the server and setup listeners if (running_ = impl_.start(); running_) @@ -789,7 +789,7 @@ GRPCServer::onStart() } void -GRPCServer::onStop() +GRPCServer::stop() { if (running_) { @@ -797,8 +797,6 @@ GRPCServer::onStop() thread_.join(); running_ = false; } - - stopped(); } GRPCServer::~GRPCServer() diff --git a/src/ripple/app/main/GRPCServer.h b/src/ripple/app/main/GRPCServer.h index 65468de733..79780e1013 100644 --- a/src/ripple/app/main/GRPCServer.h +++ b/src/ripple/app/main/GRPCServer.h @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -294,11 +293,10 @@ private: }; // GRPCServerImpl -class GRPCServer : public Stoppable +class GRPCServer { public: - explicit GRPCServer(Application& app, Stoppable& parent) - : Stoppable("GRPCServer", parent), impl_(app) + explicit GRPCServer(Application& app) : impl_(app) { } @@ -308,12 +306,12 @@ public: operator=(const GRPCServer&) = delete; void - onStart() override; + start(); void - onStop() override; + stop(); - ~GRPCServer() override; + ~GRPCServer(); private: GRPCServerImpl impl_; diff --git a/src/ripple/app/main/LoadManager.cpp b/src/ripple/app/main/LoadManager.cpp index b63972f34e..be4b7ec97f 100644 --- a/src/ripple/app/main/LoadManager.cpp +++ b/src/ripple/app/main/LoadManager.cpp @@ -30,16 +30,8 @@ namespace ripple { -LoadManager::LoadManager( - Application& app, - Stoppable& parent, - beast::Journal journal) - : Stoppable("LoadManager", parent) - , app_(app) - , journal_(journal) - , deadLock_() - , armed_(false) - , stop_(false) +LoadManager::LoadManager(Application& app, beast::Journal journal) + : app_(app), journal_(journal), deadLock_(), armed_(false) { } @@ -47,7 +39,7 @@ LoadManager::~LoadManager() { try { - onStop(); + stop(); } catch (std::exception const& ex) { @@ -78,7 +70,7 @@ LoadManager::resetDeadlockDetector() //------------------------------------------------------------------------------ void -LoadManager::onStart() +LoadManager::start() { JLOG(journal_.debug()) << "Starting"; assert(!thread_.joinable()); @@ -87,18 +79,19 @@ LoadManager::onStart() } void -LoadManager::onStop() +LoadManager::stop() { + { + std::lock_guard lock(mutex_); + stop_ = true; + // There is at most one thread waiting on this condition. + cv_.notify_all(); + } if (thread_.joinable()) { JLOG(journal_.debug()) << "Stopping"; - { - std::lock_guard sl(mutex_); - stop_ = true; - } thread_.join(); } - stopped(); } //------------------------------------------------------------------------------ @@ -109,19 +102,22 @@ LoadManager::run() beast::setCurrentThreadName("LoadManager"); using namespace std::chrono_literals; - using clock_type = std::chrono::system_clock; + using clock_type = std::chrono::steady_clock; auto t = clock_type::now(); - bool stop = false; - while (!(stop || isStopping())) + while (true) { { + t += 1s; + std::unique_lock sl(mutex_); + if (cv_.wait_until(sl, t, [this] { return stop_; })) + { + break; + } // Copy out shared data under a lock. Use copies outside lock. - std::unique_lock sl(mutex_); auto const deadLock = deadLock_; auto const armed = armed_; - stop = stop_; sl.unlock(); // Measure the amount of time we have been deadlocked, in seconds. @@ -192,30 +188,15 @@ LoadManager::run() // subscribe in NetworkOPs or Application. app_.getOPs().reportFeeChange(); } - - t += 1s; - auto const duration = t - clock_type::now(); - - if ((duration < 0s) || (duration > 1s)) - { - JLOG(journal_.warn()) << "time jump"; - t = clock_type::now(); - } - else - { - alertable_sleep_until(t); - } } - - stopped(); } //------------------------------------------------------------------------------ std::unique_ptr -make_LoadManager(Application& app, Stoppable& parent, beast::Journal journal) +make_LoadManager(Application& app, beast::Journal journal) { - return std::unique_ptr{new LoadManager{app, parent, journal}}; + return std::unique_ptr{new LoadManager{app, journal}}; } } // namespace ripple diff --git a/src/ripple/app/main/LoadManager.h b/src/ripple/app/main/LoadManager.h index c5d344bdb4..905006f5e4 100644 --- a/src/ripple/app/main/LoadManager.h +++ b/src/ripple/app/main/LoadManager.h @@ -20,7 +20,9 @@ #ifndef RIPPLE_APP_MAIN_LOADMANAGER_H_INCLUDED #define RIPPLE_APP_MAIN_LOADMANAGER_H_INCLUDED -#include +#include +#include +#include #include #include #include @@ -40,9 +42,9 @@ class Application; The warning system is used instead of merely dropping, because hostile peers can just reconnect anyway. */ -class LoadManager : public Stoppable +class LoadManager { - LoadManager(Application& app, Stoppable& parent, beast::Journal journal); + LoadManager(Application& app, beast::Journal journal); public: LoadManager() = delete; @@ -81,12 +83,11 @@ public: //-------------------------------------------------------------------------- - // Stoppable members void - onStart() override; + start(); void - onStop() override; + stop(); private: void @@ -97,22 +98,20 @@ private: beast::Journal const journal_; std::thread thread_; - std::mutex mutex_; // Guards deadLock_, armed_, and stop_. + std::mutex mutex_; // Guards deadLock_, armed_, cv_ + std::condition_variable cv_; + bool stop_ = false; std::chrono::steady_clock::time_point deadLock_; // Detect server deadlocks. bool armed_; - bool stop_; friend std::unique_ptr - make_LoadManager( - Application& app, - Stoppable& parent, - beast::Journal journal); + make_LoadManager(Application& app, beast::Journal journal); }; std::unique_ptr -make_LoadManager(Application& app, Stoppable& parent, beast::Journal journal); +make_LoadManager(Application& app, beast::Journal journal); } // namespace ripple diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index 815d342d06..c36294d89d 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -729,7 +729,7 @@ run(int argc, char** argv) return -1; // Start the server - app->doStart(true /*start timers*/); + app->start(true /*start timers*/); // Block until we get a stop RPC. app->run(); diff --git a/src/ripple/app/main/NodeStoreScheduler.cpp b/src/ripple/app/main/NodeStoreScheduler.cpp index 9d7fc22533..d94c7ee3f0 100644 --- a/src/ripple/app/main/NodeStoreScheduler.cpp +++ b/src/ripple/app/main/NodeStoreScheduler.cpp @@ -22,63 +22,27 @@ namespace ripple { -NodeStoreScheduler::NodeStoreScheduler(Stoppable& parent) - : Stoppable("NodeStoreScheduler", parent) +NodeStoreScheduler::NodeStoreScheduler(JobQueue& jobQueue) : jobQueue_(jobQueue) { } -void -NodeStoreScheduler::setJobQueue(JobQueue& jobQueue) -{ - m_jobQueue = &jobQueue; -} - -void -NodeStoreScheduler::onStop() -{ -} - -void -NodeStoreScheduler::onChildrenStopped() -{ - assert(m_taskCount == 0); - stopped(); -} - void NodeStoreScheduler::scheduleTask(NodeStore::Task& task) { - ++m_taskCount; - if (!m_jobQueue->addJob(jtWRITE, "NodeObject::store", [this, &task](Job&) { - doTask(task); + if (!jobQueue_.addJob(jtWRITE, "NodeObject::store", [&task](Job&) { + task.performScheduledTask(); })) { // Job not added, presumably because we're shutting down. // Recover by executing the task synchronously. - doTask(task); + task.performScheduledTask(); } } -void -NodeStoreScheduler::doTask(NodeStore::Task& task) -{ - task.performScheduledTask(); - - // NOTE: It feels a bit off that there are two different methods that - // call stopped(): onChildrenStopped() and doTask(). There's a - // suspicion that, as long as the Stoppable tree is configured - // correctly, this call to stopped() in doTask() can never occur. - // - // However, until we increase our confidence that the suspicion is - // correct, we will leave this code in place. - if ((--m_taskCount == 0) && isStopping() && areChildrenStopped()) - stopped(); -} - void NodeStoreScheduler::onFetch(NodeStore::FetchReport const& report) { - m_jobQueue->addLoadEvents( + jobQueue_.addLoadEvents( report.fetchType == NodeStore::FetchType::async ? jtNS_ASYNC_READ : jtNS_SYNC_READ, 1, @@ -88,7 +52,7 @@ NodeStoreScheduler::onFetch(NodeStore::FetchReport const& report) void NodeStoreScheduler::onBatchWrite(NodeStore::BatchWriteReport const& report) { - m_jobQueue->addLoadEvents(jtNS_WRITE, report.writeCount, report.elapsed); + jobQueue_.addLoadEvents(jtNS_WRITE, report.writeCount, report.elapsed); } } // namespace ripple diff --git a/src/ripple/app/main/NodeStoreScheduler.h b/src/ripple/app/main/NodeStoreScheduler.h index b37f3c3e78..533b7497b2 100644 --- a/src/ripple/app/main/NodeStoreScheduler.h +++ b/src/ripple/app/main/NodeStoreScheduler.h @@ -21,29 +21,17 @@ #define RIPPLE_APP_MAIN_NODESTORESCHEDULER_H_INCLUDED #include -#include #include #include namespace ripple { -/** A NodeStore::Scheduler which uses the JobQueue and implements the Stoppable - * API. */ -class NodeStoreScheduler : public NodeStore::Scheduler, public Stoppable +/** A NodeStore::Scheduler which uses the JobQueue. */ +class NodeStoreScheduler : public NodeStore::Scheduler { public: - NodeStoreScheduler(Stoppable& parent); + NodeStoreScheduler(JobQueue& jobQueue); - // VFALCO NOTE This is a temporary hack to solve the problem - // of circular dependency. - // - void - setJobQueue(JobQueue& jobQueue); - - void - onStop() override; - void - onChildrenStopped() override; void scheduleTask(NodeStore::Task& task) override; void @@ -52,11 +40,7 @@ public: onBatchWrite(NodeStore::BatchWriteReport const& report) override; private: - void - doTask(NodeStore::Task& task); - - JobQueue* m_jobQueue{nullptr}; - std::atomic m_taskCount{0}; + JobQueue& jobQueue_; }; } // namespace ripple diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 9ed4895f14..972b9329e0 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -224,13 +224,11 @@ public: bool start_valid, JobQueue& job_queue, LedgerMaster& ledgerMaster, - Stoppable& parent, ValidatorKeys const& validatorKeys, boost::asio::io_service& io_svc, beast::Journal journal, beast::insight::Collector::ptr const& collector) - : NetworkOPs(parent) - , app_(app) + : app_(app) , m_clock(clock) , m_journal(journal) , m_localTX(make_LocalTxs()) @@ -552,12 +550,8 @@ public: bool tryRemoveRpcSub(std::string const& strUrl) override; - //-------------------------------------------------------------------------- - // - // Stoppable. - void - onStop() override + stop() override { { boost::system::error_code ec; @@ -578,11 +572,9 @@ public: << ec.message(); } } - // Make sure that any waitHandlers pending in our timers are done - // before we declare ourselves stopped. + // Make sure that any waitHandlers pending in our timers are done. using namespace std::chrono_literals; waitHandlerCounter_.join("NetworkOPs", 1s, m_journal); - stopped(); } private: @@ -3737,15 +3729,6 @@ NetworkOPsImp::collect_metrics() counters[static_cast(OperatingMode::FULL)].transitions); } -//------------------------------------------------------------------------------ - -NetworkOPs::NetworkOPs(Stoppable& parent) - : InfoSub::Source("NetworkOPs", parent) -{ -} - -//------------------------------------------------------------------------------ - void NetworkOPsImp::StateAccounting::mode(OperatingMode om) { @@ -3794,7 +3777,6 @@ make_NetworkOPs( bool startvalid, JobQueue& job_queue, LedgerMaster& ledgerMaster, - Stoppable& parent, ValidatorKeys const& validatorKeys, boost::asio::io_service& io_svc, beast::Journal journal, @@ -3808,7 +3790,6 @@ make_NetworkOPs( startvalid, job_queue, ledgerMaster, - parent, validatorKeys, io_svc, journal, diff --git a/src/ripple/app/misc/NetworkOPs.h b/src/ripple/app/misc/NetworkOPs.h index 5a982e5d15..f49b1dd52c 100644 --- a/src/ripple/app/misc/NetworkOPs.h +++ b/src/ripple/app/misc/NetworkOPs.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -86,9 +85,6 @@ enum class OperatingMode { */ class NetworkOPs : public InfoSub::Source { -protected: - explicit NetworkOPs(Stoppable& parent); - public: using clock_type = beast::abstract_clock; @@ -102,6 +98,9 @@ public: public: ~NetworkOPs() override = default; + virtual void + stop() = 0; + //-------------------------------------------------------------------------- // // Network information @@ -282,7 +281,6 @@ make_NetworkOPs( bool start_valid, JobQueue& job_queue, LedgerMaster& ledgerMaster, - Stoppable& parent, ValidatorKeys const& validatorKeys, boost::asio::io_service& io_svc, beast::Journal journal, diff --git a/src/ripple/app/misc/SHAMapStore.h b/src/ripple/app/misc/SHAMapStore.h index eec3763aeb..7a999012c3 100644 --- a/src/ripple/app/misc/SHAMapStore.h +++ b/src/ripple/app/misc/SHAMapStore.h @@ -28,7 +28,6 @@ namespace ripple { class TransactionMaster; -class Stoppable; /** * class to create database, launch online delete thread, and @@ -43,14 +42,20 @@ public: virtual void onLedgerClosed(std::shared_ptr const& ledger) = 0; + virtual void + start() = 0; + virtual void rendezvous() const = 0; + virtual void + stop() = 0; + virtual std::uint32_t clampFetchDepth(std::uint32_t fetch_depth) const = 0; virtual std::unique_ptr - makeNodeStore(std::string const& name, std::int32_t readThreads) = 0; + makeNodeStore(std::int32_t readThreads) = 0; /** Highest ledger that may be deleted. */ virtual LedgerIndex @@ -99,7 +104,6 @@ public: std::unique_ptr make_SHAMapStore( Application& app, - Stoppable& parent, NodeStore::Scheduler& scheduler, beast::Journal journal); } // namespace ripple diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 4f70735312..4daa5f66d6 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -81,11 +81,9 @@ SHAMapStoreImp::SavedStateDB::setLastRotated(LedgerIndex seq) SHAMapStoreImp::SHAMapStoreImp( Application& app, - Stoppable& parent, NodeStore::Scheduler& scheduler, beast::Journal journal) - : Stoppable("SHAMapStore", parent) - , app_(app) + : app_(app) , scheduler_(scheduler) , journal_(journal) , working_(true) @@ -166,10 +164,8 @@ SHAMapStoreImp::SHAMapStoreImp( } std::unique_ptr -SHAMapStoreImp::makeNodeStore(std::string const& name, std::int32_t readThreads) +SHAMapStoreImp::makeNodeStore(std::int32_t readThreads) { - // Anything which calls addJob must be a descendant of the JobQueue. - // Therefore Database objects use the JobQueue as Stoppable parent. std::unique_ptr db; if (deleteInterval_) { @@ -191,10 +187,8 @@ SHAMapStoreImp::makeNodeStore(std::string const& name, std::int32_t readThreads) // Create NodeStore with two backends to allow online deletion of data auto dbr = std::make_unique( - name, scheduler_, readThreads, - app_.getJobQueue(), std::move(writableBackend), std::move(archiveBackend), app_.config().section(ConfigSection::nodeDatabase()), @@ -206,12 +200,10 @@ SHAMapStoreImp::makeNodeStore(std::string const& name, std::int32_t readThreads) else { db = NodeStore::Manager::instance().make_Database( - name, megabytes( app_.config().getValueFor(SizedItem::burstSize, std::nullopt)), scheduler_, readThreads, - app_.getJobQueue(), app_.config().section(ConfigSection::nodeDatabase()), app_.logs().journal(nodeStoreName_)); fdRequired_ += db->fdRequired(); @@ -291,7 +283,6 @@ SHAMapStoreImp::run() rendezvous_.notify_all(); if (stop_) { - stopped(); return; } cond_.wait(lock); @@ -325,7 +316,6 @@ SHAMapStoreImp::run() switch (health()) { case Health::stopping: - stopped(); return; case Health::unhealthy: continue; @@ -343,7 +333,6 @@ SHAMapStoreImp::run() switch (health()) { case Health::stopping: - stopped(); return; case Health::unhealthy: continue; @@ -359,7 +348,6 @@ SHAMapStoreImp::run() switch (health()) { case Health::stopping: - stopped(); return; case Health::unhealthy: continue; @@ -378,7 +366,6 @@ SHAMapStoreImp::run() switch (health()) { case Health::stopping: - stopped(); return; case Health::unhealthy: continue; @@ -700,23 +687,16 @@ SHAMapStoreImp::health() } void -SHAMapStoreImp::onStop() +SHAMapStoreImp::stop() { - // This is really a check for `if (thread_)`. - if (deleteInterval_) + if (thread_.joinable()) { { std::lock_guard lock(mutex_); stop_ = true; + cond_.notify_one(); } - cond_.notify_one(); - // stopped() will be called by the thread_ running run(), - // when it reaches the check for stop_. - } - else - { - // There is no thread running run(), so we must call stopped(). - stopped(); + thread_.join(); } } @@ -735,11 +715,10 @@ SHAMapStoreImp::minimumOnline() const std::unique_ptr make_SHAMapStore( Application& app, - Stoppable& parent, NodeStore::Scheduler& scheduler, beast::Journal journal) { - return std::make_unique(app, parent, scheduler, journal); + return std::make_unique(app, scheduler, journal); } } // namespace ripple diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index ea39abd6c0..6dc9c718ce 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -25,7 +25,6 @@ #include #include #include -#include #include #include #include @@ -36,7 +35,7 @@ namespace ripple { class NetworkOPs; -class SHAMapStoreImp : public Stoppable, public SHAMapStore +class SHAMapStoreImp : public SHAMapStore { private: enum Health : std::uint8_t { ok = 0, stopping, unhealthy }; @@ -124,16 +123,9 @@ private: public: SHAMapStoreImp( Application& app, - Stoppable& parent, NodeStore::Scheduler& scheduler, beast::Journal journal); - ~SHAMapStoreImp() - { - if (thread_.joinable()) - thread_.join(); - } - std::uint32_t clampFetchDepth(std::uint32_t fetch_depth) const override { @@ -142,7 +134,7 @@ public: } std::unique_ptr - makeNodeStore(std::string const& name, std::int32_t readThreads) override; + makeNodeStore(std::int32_t readThreads) override; LedgerIndex setCanDelete(LedgerIndex seq) override @@ -241,20 +233,16 @@ private: Health health(); - // - // Stoppable - // +public: void - onStart() override + start() override { if (deleteInterval_) thread_ = std::thread(&SHAMapStoreImp::run, this); } - // Called when the application begins shutdown void - onStop() override; - // Called when all child Stoppable objects have stoped + stop() override; }; } // namespace ripple diff --git a/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.cpp b/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.cpp index e9926a394d..91e85d043d 100644 --- a/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.cpp +++ b/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.cpp @@ -51,10 +51,7 @@ public: , j_(app_.journal("PgPool")) , pgPool_( #ifdef RIPPLED_REPORTING - make_PgPool( - config.section("ledger_tx_tables"), - *dynamic_cast(&app_), - j_) + make_PgPool(config.section("ledger_tx_tables"), j_) #endif ) { @@ -67,6 +64,14 @@ public: #endif } + void + stop() override + { +#ifdef RIPPLED_REPORTING + pgPool_->stop(); +#endif + } + void sweep() override; diff --git a/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.h b/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.h index 62623920b8..77193d2457 100644 --- a/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.h +++ b/src/ripple/app/rdb/backend/RelationalDBInterfacePostgres.h @@ -27,6 +27,17 @@ namespace ripple { class RelationalDBInterfacePostgres : public RelationalDBInterface { public: + /** There is only one implementation of this interface: + * RelationalDBInterfacePostgresImp. It wraps a stoppable object (PgPool) + * that does not follow RAII, and it does not go through the effort of + * following RAII either. The owner of the only object of that type + * (ApplicationImp) holds it by the type of its interface instead of its + * implementation, and thus the lifetime management methods need to be + * part of the interface. + */ + virtual void + stop() = 0; + /** * @brief sweep Sweep the database. Method is specific for postgres backend. */ diff --git a/src/ripple/app/reporting/ReportingETL.cpp b/src/ripple/app/reporting/ReportingETL.cpp index b03d78d041..4a8c4cf1de 100644 --- a/src/ripple/app/reporting/ReportingETL.cpp +++ b/src/ripple/app/reporting/ReportingETL.cpp @@ -826,9 +826,8 @@ ReportingETL::doWork() }); } -ReportingETL::ReportingETL(Application& app, Stoppable& parent) - : Stoppable("ReportingETL", parent) - , app_(app) +ReportingETL::ReportingETL(Application& app) + : app_(app) , journal_(app.journal("ReportingETL")) , publishStrand_(app_.getIOService()) , loadBalancer_(*this) diff --git a/src/ripple/app/reporting/ReportingETL.h b/src/ripple/app/reporting/ReportingETL.h index e55cbe29e4..9265f4a0ac 100644 --- a/src/ripple/app/reporting/ReportingETL.h +++ b/src/ripple/app/reporting/ReportingETL.h @@ -25,7 +25,6 @@ #include #include #include -#include #include #include #include @@ -68,7 +67,7 @@ using AccountTransactionsData = RelationalDBInterface::AccountTransactionsData; * monitoring to writing and from writing to monitoring, based on the activity * of other processes running on different machines. */ -class ReportingETL : Stoppable +class ReportingETL { private: Application& app_; @@ -267,7 +266,7 @@ private: ThreadSafeQueue>& writeQueue); public: - ReportingETL(Application& app, Stoppable& parent); + ReportingETL(Application& app); ~ReportingETL() { @@ -280,7 +279,7 @@ public: } bool - isStopping() + isStopping() const { return stopping_; } @@ -323,7 +322,7 @@ public: /// start all of the necessary components and begin ETL void - run() + start() { JLOG(journal_.info()) << "Starting reporting etl"; assert(app_.config().reporting()); @@ -336,9 +335,8 @@ public: doWork(); } - /// Stop all the necessary components void - onStop() override + stop() { JLOG(journal_.info()) << "onStop called"; JLOG(journal_.debug()) << "Stopping Reporting ETL"; @@ -351,7 +349,6 @@ public: worker_.join(); JLOG(journal_.debug()) << "Joined worker thread"; - stopped(); } ETLLoadBalancer& diff --git a/src/ripple/basics/PerfLog.h b/src/ripple/basics/PerfLog.h index 44d5c940e6..f62fd4f78a 100644 --- a/src/ripple/basics/PerfLog.h +++ b/src/ripple/basics/PerfLog.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_BASICS_PERFLOG_H #define RIPPLE_BASICS_PERFLOG_H +#include #include #include #include @@ -66,6 +67,16 @@ public: virtual ~PerfLog() = default; + virtual void + start() + { + } + + virtual void + stop() + { + } + /** * Log start of RPC call. * @@ -157,20 +168,12 @@ public: rotate() = 0; }; -} // namespace perf - -class Section; -class Stoppable; - -namespace perf { - PerfLog::Setup setup_PerfLog(Section const& section, boost::filesystem::path const& configDir); std::unique_ptr make_PerfLog( PerfLog::Setup const& setup, - Stoppable& parent, beast::Journal journal, std::function&& signalStop); diff --git a/src/ripple/basics/impl/PerfLogImp.cpp b/src/ripple/basics/impl/PerfLogImp.cpp index 7a20209b74..8f6da21664 100644 --- a/src/ripple/basics/impl/PerfLogImp.cpp +++ b/src/ripple/basics/impl/PerfLogImp.cpp @@ -303,20 +303,16 @@ PerfLogImp::report() PerfLogImp::PerfLogImp( Setup const& setup, - Stoppable& parent, beast::Journal journal, std::function&& signalStop) - : Stoppable("PerfLogImp", parent) - , setup_(setup) - , j_(journal) - , signalStop_(std::move(signalStop)) + : setup_(setup), j_(journal), signalStop_(std::move(signalStop)) { openLog(); } PerfLogImp::~PerfLogImp() { - onStop(); + stop(); } void @@ -448,14 +444,14 @@ PerfLogImp::rotate() } void -PerfLogImp::onStart() +PerfLogImp::start() { if (setup_.perfLog.size()) thread_ = std::thread(&PerfLogImp::run, this); } void -PerfLogImp::onStop() +PerfLogImp::stop() { if (thread_.joinable()) { @@ -468,12 +464,6 @@ PerfLogImp::onStop() } } -void -PerfLogImp::onChildrenStopped() -{ - stopped(); -} - //----------------------------------------------------------------------------- PerfLog::Setup @@ -501,12 +491,10 @@ setup_PerfLog(Section const& section, boost::filesystem::path const& configDir) std::unique_ptr make_PerfLog( PerfLog::Setup const& setup, - Stoppable& parent, beast::Journal journal, std::function&& signalStop) { - return std::make_unique( - setup, parent, journal, std::move(signalStop)); + return std::make_unique(setup, journal, std::move(signalStop)); } } // namespace perf diff --git a/src/ripple/basics/impl/PerfLogImp.h b/src/ripple/basics/impl/PerfLogImp.h index 0edf6b5989..412f4e2e7a 100644 --- a/src/ripple/basics/impl/PerfLogImp.h +++ b/src/ripple/basics/impl/PerfLogImp.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -67,7 +66,7 @@ struct Locked /** * Implementation class for PerfLog. */ -class PerfLogImp : public PerfLog, Stoppable +class PerfLogImp : public PerfLog { /** * Track performance counters and currently executing tasks. @@ -151,7 +150,6 @@ class PerfLogImp : public PerfLog, Stoppable public: PerfLogImp( Setup const& setup, - Stoppable& parent, beast::Journal journal, std::function&& signalStop); @@ -200,15 +198,11 @@ public: void rotate() override; - // Called when application is ready to start threads. void - onStart() override; - // Called when the application begins shutdown. + start() override; + void - onStop() override; - // Called when all child Stoppable objects have stopped. - void - onChildrenStopped() override; + stop() override; }; } // namespace perf diff --git a/src/ripple/basics/impl/ResolverAsio.cpp b/src/ripple/basics/impl/ResolverAsio.cpp index d473953dd1..f75a390304 100644 --- a/src/ripple/basics/impl/ResolverAsio.cpp +++ b/src/ripple/basics/impl/ResolverAsio.cpp @@ -218,7 +218,6 @@ public: override { assert(m_stop_called == false); - assert(m_stopped == true); assert(!names.empty()); // TODO NIKB use rvalue references to construct and move diff --git a/src/ripple/core/ClosureCounter.h b/src/ripple/core/ClosureCounter.h index 79627bc70e..7c0d36d9f3 100644 --- a/src/ripple/core/ClosureCounter.h +++ b/src/ripple/core/ClosureCounter.h @@ -29,13 +29,27 @@ namespace ripple { -// A class that does reference counting for postponed closures -- a closure -// who's execution is delayed by a timer or queue. The reference counting -// allows a Stoppable to assure that all such postponed closures are -// completed before the Stoppable declares itself stopped(). -// -// Ret_t is the type that the counted closure returns. -// Args_t are the types of the arguments that will be passed to the closure. +/** + * The role of a `ClosureCounter` is to assist in shutdown by letting callers + * wait for the completion of callbacks (of a single type signature) that they + * previously scheduled. The lifetime of a `ClosureCounter` consists of two + * phases: the initial expanding "fork" phase, and the subsequent shrinking + * "join" phase. + * + * In the fork phase, callers register a callback by passing the callback and + * receiving a substitute in return. The substitute has the same callable + * interface as the callback, and it informs the `ClosureCounter` whenever it + * is copied or destroyed, so that it can keep an accurate count of copies. + * + * The transition to the join phase is made by a call to `join`. In this + * phase, every substitute returned going forward will be empty, signaling to + * the caller that they should just drop the callback and cancel their + * asynchronous operation. `join` blocks until all existing callback + * substitutes are destroyed. + * + * \tparam Ret_t The return type of the closure. + * \tparam Args_t The argument types of the closure. + */ template class ClosureCounter { @@ -71,10 +85,10 @@ private: } // A private template class that helps count the number of closures - // in flight. This allows Stoppables to hold off declaring stopped() - // until all their postponed closures are dispatched. + // in flight. This allows callers to block until all their postponed + // closures are dispatched. template - class Wrapper + class Substitute { private: ClosureCounter& counter_; @@ -86,33 +100,33 @@ private: "Closure arguments don't match ClosureCounter Ret_t or Args_t"); public: - Wrapper() = delete; + Substitute() = delete; - Wrapper(Wrapper const& rhs) + Substitute(Substitute const& rhs) : counter_(rhs.counter_), closure_(rhs.closure_) { ++counter_; } - Wrapper(Wrapper&& rhs) noexcept( + Substitute(Substitute&& rhs) noexcept( std::is_nothrow_move_constructible::value) : counter_(rhs.counter_), closure_(std::move(rhs.closure_)) { ++counter_; } - Wrapper(ClosureCounter& counter, Closure&& closure) + Substitute(ClosureCounter& counter, Closure&& closure) : counter_(counter), closure_(std::forward(closure)) { ++counter_; } - Wrapper& - operator=(Wrapper const& rhs) = delete; - Wrapper& - operator=(Wrapper&& rhs) = delete; + Substitute& + operator=(Substitute const& rhs) = delete; + Substitute& + operator=(Substitute&& rhs) = delete; - ~Wrapper() + ~Substitute() { --counter_; } @@ -174,10 +188,10 @@ public: reference counter. */ template - std::optional> + std::optional> wrap(Closure&& closure) { - std::optional> ret; + std::optional> ret; std::lock_guard lock{mutex_}; if (!waitForClosures_) diff --git a/src/ripple/core/Job.h b/src/ripple/core/Job.h index 4cd75dbafd..a5fbf5413e 100644 --- a/src/ripple/core/Job.h +++ b/src/ripple/core/Job.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_CORE_JOB_H_INCLUDED #define RIPPLE_CORE_JOB_H_INCLUDED +#include #include #include @@ -157,6 +158,8 @@ private: clock_type::time_point m_queue_time; }; +using JobCounter = ClosureCounter; + } // namespace ripple #endif diff --git a/src/ripple/core/JobQueue.h b/src/ripple/core/JobQueue.h index 6c14b117db..1d3fd5498e 100644 --- a/src/ripple/core/JobQueue.h +++ b/src/ripple/core/JobQueue.h @@ -21,9 +21,9 @@ #define RIPPLE_CORE_JOBQUEUE_H_INCLUDED #include +#include #include #include -#include #include #include #include @@ -52,7 +52,7 @@ struct Coro_create_t When the JobQueue stops, it waits for all jobs and coroutines to finish. */ -class JobQueue : public Stoppable, private Workers::Callback +class JobQueue : private Workers::Callback { public: /** Coroutines must run to completion. */ @@ -142,7 +142,6 @@ public: JobQueue( beast::insight::Collector::ptr const& collector, - Stoppable& parent, beast::Journal journal, Logs& logs, perf::PerfLog& perfLog); @@ -165,8 +164,8 @@ public: bool addJob(JobType type, std::string const& name, JobHandler&& jobHandler) { - if (auto optionalCountedJob = Stoppable::jobCounter().wrap( - std::forward(jobHandler))) + if (auto optionalCountedJob = + jobCounter_.wrap(std::forward(jobHandler))) { return addRefCountedJob(type, name, std::move(*optionalCountedJob)); } @@ -224,10 +223,24 @@ public: Json::Value getJson(int c = 0); - /** Block until no tasks running. */ + /** Block until no jobs running. */ void rendezvous(); + void + stop(); + + bool + isStopping() const + { + return stopping_; + } + + // We may be able to move away from this, but we can keep it during the + // transition. + bool + isStopped() const; + private: friend class Coro; @@ -237,6 +250,9 @@ private: mutable std::mutex m_mutex; std::uint64_t m_lastJob; std::set m_jobSet; + JobCounter jobCounter_; + std::atomic_bool stopping_{false}; + std::atomic_bool stopped_{false}; JobDataMap m_jobData; JobTypeData m_invalidJobData; @@ -262,13 +278,6 @@ private: JobTypeData& getJobTypeData(JobType type); - void - onStop() override; - - // Signals the service stopped if the stopped condition is met. - void - checkStopped(std::lock_guard const& lock); - // Adds a reference counted job to the JobQueue. // // param type The type of job. @@ -354,9 +363,6 @@ private: // will be enough. int getJobLimit(JobType type); - - void - onChildrenStopped() override; }; /* diff --git a/src/ripple/core/Pg.cpp b/src/ripple/core/Pg.cpp index 977d8e18cd..3c75c00b3b 100644 --- a/src/ripple/core/Pg.cpp +++ b/src/ripple/core/Pg.cpp @@ -324,8 +324,7 @@ Pg::clear() //----------------------------------------------------------------------------- -PgPool::PgPool(Section const& pgConfig, Stoppable& parent, beast::Journal j) - : Stoppable("PgPool", parent), j_(j) +PgPool::PgPool(Section const& pgConfig, beast::Journal j) : j_(j) { // Make sure that boost::asio initializes the SSL library. { @@ -501,13 +500,12 @@ PgPool::setup() } void -PgPool::onStop() +PgPool::stop() { std::lock_guard lock(mutex_); stop_ = true; cond_.notify_all(); idle_.clear(); - stopped(); JLOG(j_.info()) << "stopped"; } @@ -595,9 +593,9 @@ PgPool::checkin(std::unique_ptr& pg) //----------------------------------------------------------------------------- std::shared_ptr -make_PgPool(Section const& pgConfig, Stoppable& parent, beast::Journal j) +make_PgPool(Section const& pgConfig, beast::Journal j) { - auto ret = std::make_shared(pgConfig, parent, j); + auto ret = std::make_shared(pgConfig, j); ret->setup(); return ret; } diff --git a/src/ripple/core/Pg.h b/src/ripple/core/Pg.h index 42e7761962..b2e8f465c3 100644 --- a/src/ripple/core/Pg.h +++ b/src/ripple/core/Pg.h @@ -23,7 +23,6 @@ #include #include -#include #include #include #include @@ -366,7 +365,7 @@ public: * This should be stored as a shared pointer so PgQuery objects can safely * outlive it. */ -class PgPool : public Stoppable +class PgPool { friend class PgQuery; @@ -409,9 +408,8 @@ public: * * @param pgConfig Postgres config. * @param j Logger object. - * @param parent Stoppable parent. */ - PgPool(Section const& pgConfig, Stoppable& parent, beast::Journal j); + PgPool(Section const& pgConfig, beast::Journal j); /** Initiate idle connection timer. * @@ -421,9 +419,9 @@ public: void setup(); - /** Prepare for process shutdown. (Stoppable) */ + /** Prepare for process shutdown. */ void - onStop() override; + stop(); /** Disconnect idle postgres connections. */ void @@ -501,11 +499,10 @@ public: * * @param pgConfig Configuration for Postgres. * @param j Logger object. - * @param parent Stoppable parent object. * @return Postgres connection pool manager */ std::shared_ptr -make_PgPool(Section const& pgConfig, Stoppable& parent, beast::Journal j); +make_PgPool(Section const& pgConfig, beast::Journal j); /** Initialize the Postgres schema. * diff --git a/src/ripple/core/Stoppable.h b/src/ripple/core/Stoppable.h deleted file mode 100644 index e192b83bea..0000000000 --- a/src/ripple/core/Stoppable.h +++ /dev/null @@ -1,447 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012-2015 Ripple Labs Inc. - - Permission to use, copy, modify, and/or 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. -*/ -//============================================================================== - -#ifndef RIPPLE_CORE_STOPPABLE_H_INCLUDED -#define RIPPLE_CORE_STOPPABLE_H_INCLUDED - -#include -#include -#include -#include -#include -#include -#include -#include - -namespace ripple { - -// Give a reasonable name for the JobCounter -using JobCounter = ClosureCounter; - -class RootStoppable; - -/** Provides an interface for starting and stopping. - - A common method of structuring server or peer to peer code is to isolate - conceptual portions of functionality into individual classes, aggregated - into some larger "application" or "core" object which holds all the parts. - Frequently, these components are dependent on each other in unavoidably - complex ways. They also often use threads and perform asynchronous i/o - operations involving sockets or other operating system objects. The process - of starting and stopping such a system can be complex. This interface - provides a set of behaviors for ensuring that the start and stop of a - composite application-style object is well defined. - - Upon the initialization of the composite object these steps are performed: - - 1. Construct sub-components. - - These are all typically derived from Stoppable. There can be a deep - hierarchy: Stoppable objects may themselves have Stoppable child - objects. This captures the relationship of dependencies. - - 2. prepare() - - Because some components may depend on others, preparatory steps require - that all objects be first constructed. The prepare step calls all - Stoppable objects in the tree starting from the leaves and working up - to the root. In this stage we are guaranteed that all objects have been - constructed and are in a well-defined state. - - 3. onPrepare() - - This override is called for all Stoppable objects in the hierarchy - during the prepare stage. It is guaranteed that all child Stoppable - objects have already been prepared when this is called. - - Objects are called children first. - - 4. start() - - At this point all sub-components have been constructed and prepared, - so it should be safe for them to be started. While some Stoppable - objects may do nothing in their start function, others will start - threads or call asynchronous i/o initiating functions like timers or - sockets. - - 5. onStart() - - This override is called for all Stoppable objects in the hierarchy - during the start stage. It is guaranteed that no child Stoppable - objects have been started when this is called. - - Objects are called parent first. - - This is the sequence of events involved in stopping: - - 6. stopAsync() [optional] - - This notifies the root Stoppable and all its children that a stop is - requested. - - 7. stop() - - This first calls stopAsync(), and then blocks on each child Stoppable in - the in the tree from the bottom up, until the Stoppable indicates it has - stopped. This will usually be called from the main thread of execution - when some external signal indicates that the process should stop. For - example, an RPC 'stop' command, or a SIGINT POSIX signal. - - 8. onStop() - - This override is called for the root Stoppable and all its children when - stopAsync() is called. Derived classes should cancel pending I/O and - timers, signal that threads should exit, queue cleanup jobs, and perform - any other necessary final actions in preparation for exit. - - Objects are called parent first. - - 9. onChildrenStopped() - - This override is called when all the children have stopped. This informs - the Stoppable that there should not be any more dependents making calls - into its member functions. A Stoppable that has no children will still - have this function called. - - Objects are called children first. - - 10. stopped() - - The derived class calls this function to inform the Stoppable API that - it has completed the stop. This unblocks the caller of stop(). - - For stoppables which are only considered stopped when all of their - children have stopped, and their own internal logic indicates a stop, it - will be necessary to perform special actions in onChildrenStopped(). The - function areChildrenStopped() can be used after children have stopped, - but before the Stoppable logic itself has stopped, to determine if the - stoppable's logic is a true stop. - - Pseudo code for this process is as follows: - - @code - - // Returns `true` if derived logic has stopped. - // - // When the logic stops, logicProcessingStop() is no longer called. - // If children are still active we need to wait until we get a - // notification that the children have stopped. - // - bool logicHasStopped (); - - // Called when children have stopped - void onChildrenStopped () - { - // We have stopped when the derived logic stops and children stop. - if (logicHasStopped) - stopped(); - } - - // derived-specific logic that executes periodically - void logicProcessingStep () - { - // process - // ... - - // now see if we've stopped - if (logicHasStopped() && areChildrenStopped()) - stopped(); - } - - @endcode - - Derived class that manage one or more threads should typically notify - those threads in onStop that they should exit. In the thread function, - when the last thread is about to exit it would call stopped(). - - @note A Stoppable may not be restarted. - - The form of the Stoppable tree in the rippled application evolves as - the source code changes and reacts to new demands. As of July in 2020 - the Stoppable tree had this form: - - @code - - Application - | - +--------------------+--------------------+ - | | | - LoadManager SHAMapStore NodeStoreScheduler - | - JobQueue - | - +------+---------+--------+----------+---+-----------+--+---+ - | | | | | | | | - | NetworkOPs | InboundLedgers | | OrderbookDB | - | | | GRPCServer | - | | | Database - Overlay InboundTransactions LedgerMaster | - | | | - PeerFinder LedgerCleaner TaskQueue - - @endcode -*/ -/** @{ */ -class Stoppable -{ -protected: - Stoppable(std::string name, RootStoppable& root); - -public: - /** Create the Stoppable. */ - Stoppable(std::string name, Stoppable& parent); - - /** Destroy the Stoppable. */ - virtual ~Stoppable(); - - RootStoppable& - getRoot() - { - return m_root; - } - - /** Set the parent of this Stoppable. - - @note The Stoppable must not already have a parent. - The parent to be set cannot not be stopping. - Both roots must match. - */ - void - setParent(Stoppable& parent); - - /** Returns `true` if the stoppable should stop. */ - bool - isStopping() const; - - /** Returns `true` if the requested stop has completed. */ - bool - isStopped() const; - - /** Returns `true` if all children have stopped. */ - bool - areChildrenStopped() const; - - /* JobQueue uses this method for Job counting. */ - inline JobCounter& - jobCounter(); - - /** Sleep or wake up on stop. - - @return `true` if we are stopping - */ - bool - alertable_sleep_until(std::chrono::system_clock::time_point const& t); - -protected: - /** Called by derived classes to indicate that the stoppable has stopped. */ - void - stopped(); - -private: - /** Override called during preparation. - Since all other Stoppable objects in the tree have already been - constructed, this provides an opportunity to perform initialization - which depends on calling into other Stoppable objects. This call is made - on the same thread that called prepare(). The default implementation does - nothing. Guaranteed to only be called once. - */ - virtual void - onPrepare(); - - /** Override called during start. */ - virtual void - onStart(); - - /** Override called when the stop notification is issued. - - The call is made on an unspecified, implementation-specific thread. - onStop and onChildrenStopped will never be called concurrently, across - all Stoppable objects descended from the same root, inclusive of the - root. - - It is safe to call isStopping, isStopped, and areChildrenStopped from - within this function; The values returned will always be valid and never - change during the callback. - - The default implementation simply calls stopped(). This is applicable - when the Stoppable has a trivial stop operation (or no stop operation), - and we are merely using the Stoppable API to position it as a dependency - of some parent service. - - Thread safety: - May not block for long periods. - Guaranteed only to be called once. - Must be safe to call from any thread at any time. - */ - virtual void - onStop(); - - /** Override called when all children have stopped. - - The call is made on an unspecified, implementation-specific thread. - onStop and onChildrenStopped will never be called concurrently, across - all Stoppable objects descended from the same root, inclusive of the - root. - - It is safe to call isStopping, isStopped, and areChildrenStopped from - within this function; The values returned will always be valid and never - change during the callback. - - The default implementation does nothing. - - Thread safety: - May not block for long periods. - Guaranteed only to be called once. - Must be safe to call from any thread at any time. - */ - virtual void - onChildrenStopped(); - - friend class RootStoppable; - - struct Child; - using Children = beast::LockFreeStack; - - struct Child : Children::Node - { - Child(Stoppable* stoppable_) : stoppable(stoppable_) - { - } - - Stoppable* stoppable; - }; - - void - prepareRecursive(); - void - startRecursive(); - void - stopAsyncRecursive(beast::Journal j); - void - stopRecursive(beast::Journal j); - - std::string m_name; - RootStoppable& m_root; - Child m_child; - // TODO [C++20]: Use std::atomic_flag instead. - std::atomic m_stopped{false}; - std::atomic m_childrenStopped{false}; - Children m_children; - std::condition_variable m_cv; - std::mutex m_mut; - bool m_is_stopping = false; - bool hasParent_{false}; -}; - -//------------------------------------------------------------------------------ - -class RootStoppable : public Stoppable -{ -public: - explicit RootStoppable(std::string name); - - virtual ~RootStoppable(); - - bool - isStopping() const; - - /** Prepare and start all contained Stoppable objects. - This calls onPrepare for all Stoppable objects in the tree, bottom-up, - then calls onStart for the same, top-down. - Calls made after the first have no effect. - Thread safety: - May be called from any thread. - */ - void - start(); - - /** Notify a root stoppable and children to stop, and block until stopped. - Has no effect if the stoppable was already notified. - This blocks until the stoppable and all of its children have stopped. - Undefined behavior results if stop() is called without finishing - a previous call to start(). - Thread safety: - Safe to call from any thread not associated with a Stoppable. - */ - void - stop(beast::Journal j); - - /** Return true if start() was ever called. */ - bool - started() const - { - return startExited_; - } - - /* JobQueue uses this method for Job counting. */ - JobCounter& - jobCounter() - { - return jobCounter_; - } - - /** Sleep or wake up on stop. - - @return `true` if we are stopping - */ - bool - alertable_sleep_until(std::chrono::system_clock::time_point const& t); - -private: - // TODO [C++20]: Use std::atomic_flag instead. - std::atomic startEntered_{false}; - std::atomic startExited_{false}; - std::atomic stopEntered_{false}; - std::mutex m_; - std::condition_variable c_; - JobCounter jobCounter_; -}; -/** @} */ - -//------------------------------------------------------------------------------ - -JobCounter& -Stoppable::jobCounter() -{ - return m_root.jobCounter(); -} - -//------------------------------------------------------------------------------ - -inline bool -RootStoppable::alertable_sleep_until( - std::chrono::system_clock::time_point const& t) -{ - std::unique_lock lock(m_); - if (stopEntered_) - return true; - // TODO [C++20]: When `stopEntered_` is changed to a `std::atomic_flag`, - // this call to `load` needs to change to a call to `test`. - return c_.wait_until(lock, t, [this] { return stopEntered_.load(); }); -} - -inline bool -Stoppable::alertable_sleep_until(std::chrono::system_clock::time_point const& t) -{ - return m_root.alertable_sleep_until(t); -} - -} // namespace ripple - -#endif diff --git a/src/ripple/core/impl/JobQueue.cpp b/src/ripple/core/impl/JobQueue.cpp index 2e1dfaa66c..4fd394994a 100644 --- a/src/ripple/core/impl/JobQueue.cpp +++ b/src/ripple/core/impl/JobQueue.cpp @@ -20,22 +20,21 @@ #include #include #include +#include namespace ripple { JobQueue::JobQueue( beast::insight::Collector::ptr const& collector, - Stoppable& parent, beast::Journal journal, Logs& logs, perf::PerfLog& perfLog) - : Stoppable("JobQueue", parent) - , m_journal(journal) + : m_journal(journal) , m_lastJob(0) , m_invalidJobData(JobTypes::instance().getInvalid(), collector, logs) , m_processCount(0) , m_workers(*this, &perfLog, "JobQueue", 0) - , m_cancelCallback(std::bind(&Stoppable::isStopping, this)) + , m_cancelCallback(std::bind(&JobQueue::isStopping, this)) , perfLog_(perfLog) , m_collector(collector) { @@ -96,24 +95,8 @@ JobQueue::addRefCountedJob( { std::lock_guard lock(m_mutex); - - // If this goes off it means that a child didn't follow - // the Stoppable API rules. A job may only be added if: - // - // - The JobQueue has NOT stopped - // AND - // * We are currently processing jobs - // OR - // * We have have pending jobs - // OR - // * Not all children are stopped - // - assert( - !isStopped() && - (m_processCount > 0 || !m_jobSet.empty() || !areChildrenStopped())); - - std::pair::iterator, bool> result(m_jobSet.insert( - Job(type, name, ++m_lastJob, data.load(), func, m_cancelCallback))); + auto result = m_jobSet.emplace( + type, name, ++m_lastJob, data.load(), func, m_cancelCallback); queueJob(*result.first, lock); } return true; @@ -278,7 +261,7 @@ void JobQueue::rendezvous() { std::unique_lock lock(m_mutex); - cv_.wait(lock, [&] { return m_processCount == 0 && m_jobSet.empty(); }); + cv_.wait(lock, [this] { return m_processCount == 0 && m_jobSet.empty(); }); } JobTypeData& @@ -296,28 +279,31 @@ JobQueue::getJobTypeData(JobType type) } void -JobQueue::onStop() +JobQueue::stop() { - // onStop must be defined and empty here, - // otherwise the base class will do the wrong thing. + stopping_ = true; + using namespace std::chrono_literals; + jobCounter_.join("JobQueue", 1s, m_journal); + { + // After the JobCounter is joined, all jobs have finished executing + // (i.e. returned from `Job::doJob`) and no more are being accepted, + // but there may still be some threads between the return of + // `Job::doJob` and the return of `JobQueue::processTask`. That is why + // we must wait on the condition variable to make these assertions. + std::unique_lock lock(m_mutex); + cv_.wait( + lock, [this] { return m_processCount == 0 && m_jobSet.empty(); }); + assert(m_processCount == 0); + assert(m_jobSet.empty()); + assert(nSuspend_ == 0); + stopped_ = true; + } } -void -JobQueue::checkStopped(std::lock_guard const& lock) +bool +JobQueue::isStopped() const { - // We are stopped when all of the following are true: - // - // 1. A stop notification was received - // 2. All Stoppable children have stopped - // 3. There are no executing calls to processTask - // 4. There are no remaining Jobs in the job set - // 5. There are no suspended coroutines - // - if (isStopping() && areChildrenStopped() && (m_processCount == 0) && - m_jobSet.empty() && nSuspend_ == 0) - { - stopped(); - } + return stopped_; } void @@ -437,13 +423,12 @@ JobQueue::processTask(int instance) { std::lock_guard lock(m_mutex); - // Job should be destroyed before calling checkStopped + // Job should be destroyed before stopping // otherwise destructors with side effects can access // parent objects that are already destroyed. finishJob(type); if (--m_processCount == 0 && m_jobSet.empty()) cv_.notify_all(); - checkStopped(lock); } // Note that when Job::~Job is called, the last reference @@ -459,11 +444,4 @@ JobQueue::getJobLimit(JobType type) return j.limit(); } -void -JobQueue::onChildrenStopped() -{ - std::lock_guard lock(m_mutex); - checkStopped(lock); -} - } // namespace ripple diff --git a/src/ripple/core/impl/Stoppable.cpp b/src/ripple/core/impl/Stoppable.cpp deleted file mode 100644 index 5d54c15660..0000000000 --- a/src/ripple/core/impl/Stoppable.cpp +++ /dev/null @@ -1,221 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. - - Permission to use, copy, modify, and/or 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 -#include - -#include - -namespace ripple { - -Stoppable::Stoppable(std::string name, RootStoppable& root) - : m_name(std::move(name)), m_root(root), m_child(this) -{ -} - -Stoppable::Stoppable(std::string name, Stoppable& parent) - : m_name(std::move(name)), m_root(parent.m_root), m_child(this) -{ - setParent(parent); -} - -Stoppable::~Stoppable() -{ -} - -void -Stoppable::setParent(Stoppable& parent) -{ - assert(!hasParent_); - assert(!parent.isStopping()); - assert(std::addressof(m_root) == std::addressof(parent.m_root)); - - parent.m_children.push_front(&m_child); - hasParent_ = true; -} - -bool -Stoppable::isStopping() const -{ - return m_root.isStopping(); -} - -bool -Stoppable::isStopped() const -{ - return m_stopped; -} - -bool -Stoppable::areChildrenStopped() const -{ - return m_childrenStopped; -} - -void -Stoppable::stopped() -{ - std::lock_guard lk{m_mut}; - m_is_stopping = true; - m_cv.notify_all(); -} - -void -Stoppable::onPrepare() -{ -} - -void -Stoppable::onStart() -{ -} - -void -Stoppable::onStop() -{ - stopped(); -} - -void -Stoppable::onChildrenStopped() -{ -} - -//------------------------------------------------------------------------------ - -void -Stoppable::prepareRecursive() -{ - for (Children::const_iterator iter(m_children.cbegin()); - iter != m_children.cend(); - ++iter) - iter->stoppable->prepareRecursive(); - onPrepare(); -} - -void -Stoppable::startRecursive() -{ - onStart(); - for (Children::const_iterator iter(m_children.cbegin()); - iter != m_children.cend(); - ++iter) - iter->stoppable->startRecursive(); -} - -void -Stoppable::stopAsyncRecursive(beast::Journal j) -{ - onStop(); - - for (Children::const_iterator iter(m_children.cbegin()); - iter != m_children.cend(); - ++iter) - iter->stoppable->stopAsyncRecursive(j); -} - -void -Stoppable::stopRecursive(beast::Journal j) -{ - // Block on each child from the bottom of the tree up. - // - for (Children::const_iterator iter(m_children.cbegin()); - iter != m_children.cend(); - ++iter) - iter->stoppable->stopRecursive(j); - - // if we get here then all children have stopped - // - m_childrenStopped = true; - onChildrenStopped(); - - // Now block on this Stoppable until m_is_stopping is set by stopped(). - // - using namespace std::chrono_literals; - std::unique_lock lk{m_mut}; - if (!m_cv.wait_for(lk, 1s, [this] { return m_is_stopping; })) - { - if (auto stream = j.error()) - stream << "Waiting for '" << m_name << "' to stop"; - m_cv.wait(lk, [this] { return m_is_stopping; }); - } - m_stopped = true; -} - -//------------------------------------------------------------------------------ - -RootStoppable::RootStoppable(std::string name) - : Stoppable(std::move(name), *this) -{ -} - -RootStoppable::~RootStoppable() -{ - using namespace std::chrono_literals; - jobCounter_.join(m_name.c_str(), 1s, debugLog()); -} - -bool -RootStoppable::isStopping() const -{ - return stopEntered_; -} - -void -RootStoppable::start() -{ - if (startEntered_.exchange(true)) - return; - prepareRecursive(); - startRecursive(); - startExited_ = true; -} - -void -RootStoppable::stop(beast::Journal j) -{ - // Must have a prior call to start() - assert(startExited_); - - bool alreadyCalled; - { - // Even though stopEntered_ is atomic, we change its value under a - // lock. This removes a small timing window that occurs if the - // waiting thread is handling a spurious wakeup while stopEntered_ - // changes state. - std::unique_lock lock(m_); - alreadyCalled = stopEntered_.exchange(true); - } - if (alreadyCalled) - { - if (auto stream = j.warn()) - stream << "RootStoppable::stop called again"; - return; - } - - // Wait until all in-flight JobQueue Jobs are completed. - using namespace std::chrono_literals; - jobCounter_.join(m_name.c_str(), 1s, j); - - c_.notify_all(); - stopAsyncRecursive(j); - stopRecursive(j); -} - -} // namespace ripple diff --git a/src/ripple/core/impl/Workers.cpp b/src/ripple/core/impl/Workers.cpp index 321ca36856..732e6f0ec8 100644 --- a/src/ripple/core/impl/Workers.cpp +++ b/src/ripple/core/impl/Workers.cpp @@ -44,7 +44,7 @@ Workers::Workers( Workers::~Workers() { - pauseAllThreadsAndWait(); + stop(); deleteWorkers(m_everyone); } @@ -111,7 +111,7 @@ Workers::setNumberOfThreads(int numberOfThreads) } void -Workers::pauseAllThreadsAndWait() +Workers::stop() { setNumberOfThreads(0); diff --git a/src/ripple/core/impl/Workers.h b/src/ripple/core/impl/Workers.h index f54093fba8..0abbbe429c 100644 --- a/src/ripple/core/impl/Workers.h +++ b/src/ripple/core/impl/Workers.h @@ -34,7 +34,36 @@ namespace perf { class PerfLog; } -/** A group of threads that process tasks. +/** + * `Workers` is effectively a thread pool. The constructor takes a "callback" + * that has a `void processTask(int instance)` method, and a number of + * workers. It creates that many Workers and then waits for calls to + * `Workers::addTask()`. It holds a semaphore that counts the number of + * waiting tasks, and a condition variable for the event when the last worker + * pauses itself. + * + * Creating a `Worker` creates a thread that calls `Worker::run()`. When that + * thread enters `Worker::run`, it increments the count of active workers in + * the parent `Workers` object and then blocks on the semaphore if there are + * no waiting tasks. It will be unblocked whenever the number of waiting tasks + * is incremented. That only happens in two circumstances: (1) when + * `Workers::addTask` is called and (2) when `Workers` wants to pause some + * workers ("pause one worker" is considered one task), which happens when + * someone wants to stop the workers or shrink the threadpool. No worker + * threads are ever destroyed until `Workers` is destroyed; it merely pauses + * workers until then. + * + * When an idle worker is woken, it checks whether `Workers` is trying to pause + * workers. If so, it adds itself to the set of paused workers and blocks on + * its own condition variable. If not, then it calls `processTask` on the + * "callback" held by `Workers`. + * + * When a paused worker is woken, it checks whether it should exit. The signal + * to exit is only set in the destructor of `Worker`, which unblocks the + * paused thread and waits for it to exit. A `Worker::run` thread checks + * whether it needs to exit only when it is woken from a pause (not when it is + * woken from idle). This is why the destructor for `Workers` pauses all the + * workers before destroying them. */ class Workers { @@ -104,7 +133,7 @@ public: @note This function is not thread-safe. */ void - pauseAllThreadsAndWait(); + stop(); /** Add a task to be performed. diff --git a/src/ripple/net/HTTPDownloader.h b/src/ripple/net/HTTPDownloader.h index 971627e2f0..1f2243a4fe 100644 --- a/src/ripple/net/HTTPDownloader.h +++ b/src/ripple/net/HTTPDownloader.h @@ -57,7 +57,7 @@ public: bool ssl = true); void - onStop(); + stop(); virtual ~HTTPDownloader() = default; diff --git a/src/ripple/net/InfoSub.h b/src/ripple/net/InfoSub.h index b1ef6e25be..c776e6f99b 100644 --- a/src/ripple/net/InfoSub.h +++ b/src/ripple/net/InfoSub.h @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -53,12 +52,11 @@ public: public: /** Abstracts the source of subscription data. */ - class Source : public Stoppable + class Source { - protected: - Source(char const* name, Stoppable& parent); - public: + virtual ~Source() = default; + // For some reason, these were originally called "rt" // for "real time". They actually refer to whether // you get transactions as they occur or once their diff --git a/src/ripple/net/RPCSub.h b/src/ripple/net/RPCSub.h index e62aeeb4ca..e2193b1ce1 100644 --- a/src/ripple/net/RPCSub.h +++ b/src/ripple/net/RPCSub.h @@ -21,7 +21,6 @@ #define RIPPLE_NET_RPCSUB_H_INCLUDED #include -#include #include #include diff --git a/src/ripple/net/ShardDownloader.md b/src/ripple/net/ShardDownloader.md index ded58bbe22..9d8a33ae40 100644 --- a/src/ripple/net/ShardDownloader.md +++ b/src/ripple/net/ShardDownloader.md @@ -58,7 +58,7 @@ Much of the shard downloading process concerns the following classes: file, or simply return if there are no more downloads to process. When `complete` is invoked with no remaining files to be downloaded, the handler and downloader are not destroyed automatically, but persist for the duration - of the application to assist with graceful shutdowns by `Stoppable`. + of the application to assist with graceful shutdowns. - `DatabaseBody` @@ -86,18 +86,18 @@ std::atomic stop_; ##### Thread 1: -A graceful shutdown begins when the `onStop()` method of the +A graceful shutdown begins when the `stop()` method of the `ShardArchiveHandler` is invoked: ```c++ void -ShardArchiveHandler::onStop() +ShardArchiveHandler::stop() { std::lock_guard lock(m_); if (downloader_) { - downloader_->onStop(); + downloader_->stop(); downloader_.reset(); } @@ -105,13 +105,13 @@ ShardArchiveHandler::onStop() } ``` -Inside of `HTTPDownloader::onStop()`, if a download is currently in progress, +Inside of `HTTPDownloader::stop()`, if a download is currently in progress, the `stop_` member variable is set and the thread waits for the download to stop: ```c++ void -HTTPDownloader::onStop() +HTTPDownloader::stop() { std::unique_lock lock(m_); diff --git a/src/ripple/net/impl/HTTPDownloader.cpp b/src/ripple/net/impl/HTTPDownloader.cpp index 3e3b511a75..5ed2ceae0d 100644 --- a/src/ripple/net/impl/HTTPDownloader.cpp +++ b/src/ripple/net/impl/HTTPDownloader.cpp @@ -123,8 +123,9 @@ HTTPDownloader::do_session( // When the downloader is being stopped // because the server is shutting down, - // this method notifies a 'Stoppable' - // object that the session has ended. + // this method notifies a caller of `onStop` + // (`RPC::ShardArchiveHandler` to be specific) + // that the session has ended. auto exit = [this, &dstPath, complete] { if (!stop_) complete(std::move(dstPath)); @@ -280,7 +281,7 @@ HTTPDownloader::do_session( } void -HTTPDownloader::onStop() +HTTPDownloader::stop() { stop_ = true; diff --git a/src/ripple/net/impl/InfoSub.cpp b/src/ripple/net/impl/InfoSub.cpp index a13b7f736b..19cdb5364d 100644 --- a/src/ripple/net/impl/InfoSub.cpp +++ b/src/ripple/net/impl/InfoSub.cpp @@ -33,15 +33,6 @@ namespace ripple { // code assumes this node is synched (and will continue to do so until // there's a functional network. -//------------------------------------------------------------------------------ - -InfoSub::Source::Source(char const* name, Stoppable& parent) - : Stoppable(name, parent) -{ -} - -//------------------------------------------------------------------------------ - InfoSub::InfoSub(Source& source) : m_source(source), mSeq(assign_id()) { } diff --git a/src/ripple/nodestore/Database.h b/src/ripple/nodestore/Database.h index 25c4702e3f..861378551e 100644 --- a/src/ripple/nodestore/Database.h +++ b/src/ripple/nodestore/Database.h @@ -20,12 +20,14 @@ #ifndef RIPPLE_NODESTORE_DATABASE_H_INCLUDED #define RIPPLE_NODESTORE_DATABASE_H_INCLUDED -#include +#include +#include #include #include #include #include +#include #include namespace ripple { @@ -47,23 +49,19 @@ namespace NodeStore { @see NodeObject */ -class Database : public Stoppable +class Database { public: Database() = delete; /** Construct the node store. - @param name The Stoppable name for this Database. - @param parent The parent Stoppable. @param scheduler The scheduler to use for performing asynchronous tasks. @param readThreads The number of asynchronous read threads to create. @param config The configuration settings @param journal Destination for logging output. */ Database( - std::string name, - Stoppable& parent, Scheduler& scheduler, int readThreads, Section const& config, @@ -220,11 +218,11 @@ public: return fdRequired_; } - void - onStop() override; + virtual void + stop(); - void - onChildrenStopped() override; + bool + isStopping() const; /** @return The earliest ledger sequence allowed */ @@ -242,9 +240,6 @@ protected: std::atomic fetchHitCount_{0}; std::atomic fetchSz_{0}; - void - stopReadThreads(); - void storeStats(std::uint64_t count, std::uint64_t sz) { @@ -276,7 +271,7 @@ private: std::atomic fetchDurationUs_{0}; std::atomic storeDurationUs_{0}; - std::mutex readLock_; + mutable std::mutex readLock_; std::condition_variable readCondVar_; // reads to do @@ -291,7 +286,7 @@ private: uint256 readLastHash_; std::vector readThreads_; - bool readShut_{false}; + bool readStopping_{false}; // The default is 32570 to match the XRP ledger network's earliest // allowed sequence. Alternate networks may set this value. diff --git a/src/ripple/nodestore/DatabaseRotating.h b/src/ripple/nodestore/DatabaseRotating.h index 36d7041202..c8d7104fe5 100644 --- a/src/ripple/nodestore/DatabaseRotating.h +++ b/src/ripple/nodestore/DatabaseRotating.h @@ -34,13 +34,11 @@ class DatabaseRotating : public Database { public: DatabaseRotating( - std::string const& name, - Stoppable& parent, Scheduler& scheduler, int readThreads, Section const& config, beast::Journal journal) - : Database(name, parent, scheduler, readThreads, config, journal) + : Database(scheduler, readThreads, config, journal) { } diff --git a/src/ripple/nodestore/DatabaseShard.h b/src/ripple/nodestore/DatabaseShard.h index 4c1efdfa5d..1c71f073c8 100644 --- a/src/ripple/nodestore/DatabaseShard.h +++ b/src/ripple/nodestore/DatabaseShard.h @@ -39,21 +39,17 @@ class DatabaseShard : public Database public: /** Construct a shard store - @param name The Stoppable name for this Database - @param parent The parent Stoppable @param scheduler The scheduler to use for performing asynchronous tasks @param readThreads The number of asynchronous read threads to create @param config The shard configuration section for the database @param journal Destination for logging output */ DatabaseShard( - std::string const& name, - Stoppable& parent, Scheduler& scheduler, int readThreads, Section const& config, beast::Journal journal) - : Database(name, parent, scheduler, readThreads, config, journal) + : Database(scheduler, readThreads, config, journal) { } @@ -287,7 +283,6 @@ seqToShardIndex( extern std::unique_ptr make_ShardStore( Application& app, - Stoppable& parent, Scheduler& scheduler, int readThreads, beast::Journal j); diff --git a/src/ripple/nodestore/Manager.h b/src/ripple/nodestore/Manager.h index e7b4ecec91..d28fd2bcaf 100644 --- a/src/ripple/nodestore/Manager.h +++ b/src/ripple/nodestore/Manager.h @@ -95,11 +95,9 @@ public: */ virtual std::unique_ptr make_Database( - std::string const& name, std::size_t burstSize, Scheduler& scheduler, int readThreads, - Stoppable& parent, Section const& backendParameters, beast::Journal journal) = 0; }; diff --git a/src/ripple/nodestore/impl/Database.cpp b/src/ripple/nodestore/impl/Database.cpp index 8c08b6cf2e..1731c0dd2f 100644 --- a/src/ripple/nodestore/impl/Database.cpp +++ b/src/ripple/nodestore/impl/Database.cpp @@ -30,14 +30,11 @@ namespace ripple { namespace NodeStore { Database::Database( - std::string name, - Stoppable& parent, Scheduler& scheduler, int readThreads, Section const& config, beast::Journal journal) - : Stoppable(name, parent.getRoot()) - , j_(journal) + : j_(journal) , scheduler_(scheduler) , earliestLedgerSeq_( get(config, "earliest_seq", XRP_LEDGER_EARLIEST_SEQ)) @@ -52,37 +49,32 @@ Database::Database( Database::~Database() { // NOTE! - // Any derived class should call the stopReadThreads() method in its + // Any derived class should call the stop() method in its // destructor. Otherwise, occasionally, the derived class may // crash during shutdown when its members are accessed by one of // these threads after the derived class is destroyed but before // this base class is destroyed. - stopReadThreads(); + stop(); +} + +bool +Database::isStopping() const +{ + std::lock_guard lock(readLock_); + return readStopping_; } void -Database::onStop() +Database::stop() { // After stop time we can no longer use the JobQueue for background // reads. Join the background read threads. - stopReadThreads(); -} - -void -Database::onChildrenStopped() -{ - stopped(); -} - -void -Database::stopReadThreads() -{ { std::lock_guard lock(readLock_); - if (readShut_) // Only stop threads once. + if (readStopping_) // Only stop threads once. return; - readShut_ = true; + readStopping_ = true; readCondVar_.notify_all(); } @@ -280,12 +272,9 @@ Database::threadEntry() { std::unique_lock lock(readLock_); - while (!readShut_ && read_.empty()) - { - // All work is done - readCondVar_.wait(lock); - } - if (readShut_) + readCondVar_.wait( + lock, [this] { return readStopping_ || !read_.empty(); }); + if (readStopping_) break; // Read in key order to make the back end more efficient diff --git a/src/ripple/nodestore/impl/DatabaseNodeImp.h b/src/ripple/nodestore/impl/DatabaseNodeImp.h index 3c6309e393..c479082b07 100644 --- a/src/ripple/nodestore/impl/DatabaseNodeImp.h +++ b/src/ripple/nodestore/impl/DatabaseNodeImp.h @@ -36,15 +36,12 @@ public: operator=(DatabaseNodeImp const&) = delete; DatabaseNodeImp( - std::string const& name, Scheduler& scheduler, int readThreads, - Stoppable& parent, std::shared_ptr backend, Section const& config, beast::Journal j) - : Database(name, parent, scheduler, readThreads, config, j) - , cache_(nullptr) + : Database(scheduler, readThreads, config, j) , backend_(std::move(backend)) { std::optional cacheSize, cacheAge; @@ -73,20 +70,18 @@ public: if (!cacheAge || *cacheAge == 0) cacheAge = 5; cache_ = std::make_shared>( - name, + "DatabaseNodeImp", cacheSize.value(), std::chrono::minutes{cacheAge.value()}, stopwatch(), j); } assert(backend_); - setParent(parent); } - ~DatabaseNodeImp() override + ~DatabaseNodeImp() { - // Stop read threads in base before data members are destroyed - stopReadThreads(); + stop(); } std::string diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp index 034efb0e67..24ddb61859 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp @@ -25,15 +25,13 @@ namespace ripple { namespace NodeStore { DatabaseRotatingImp::DatabaseRotatingImp( - std::string const& name, Scheduler& scheduler, int readThreads, - Stoppable& parent, std::shared_ptr writableBackend, std::shared_ptr archiveBackend, Section const& config, beast::Journal j) - : DatabaseRotating(name, parent, scheduler, readThreads, config, j) + : DatabaseRotating(scheduler, readThreads, config, j) , writableBackend_(std::move(writableBackend)) , archiveBackend_(std::move(archiveBackend)) { @@ -41,7 +39,6 @@ DatabaseRotatingImp::DatabaseRotatingImp( fdRequired_ += writableBackend_->fdRequired(); if (archiveBackend_) fdRequired_ += archiveBackend_->fdRequired(); - setParent(parent); } void diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.h b/src/ripple/nodestore/impl/DatabaseRotatingImp.h index 304eb6a58a..44b257dd01 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.h +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.h @@ -34,19 +34,16 @@ public: operator=(DatabaseRotatingImp const&) = delete; DatabaseRotatingImp( - std::string const& name, Scheduler& scheduler, int readThreads, - Stoppable& parent, std::shared_ptr writableBackend, std::shared_ptr archiveBackend, Section const& config, beast::Journal j); - ~DatabaseRotatingImp() override + ~DatabaseRotatingImp() { - // Stop read threads in base before data members are destroyed - stopReadThreads(); + stop(); } void diff --git a/src/ripple/nodestore/impl/DatabaseShardImp.cpp b/src/ripple/nodestore/impl/DatabaseShardImp.cpp index c3e337ef7f..60bc7361df 100644 --- a/src/ripple/nodestore/impl/DatabaseShardImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseShardImp.cpp @@ -43,21 +43,15 @@ namespace NodeStore { DatabaseShardImp::DatabaseShardImp( Application& app, - Stoppable& parent, - std::string const& name, Scheduler& scheduler, int readThreads, beast::Journal j) : DatabaseShard( - name, - parent, scheduler, readThreads, app.config().section(ConfigSection::shardDatabase()), j) , app_(app) - , parent_(parent) - , taskQueue_(std::make_unique(*this)) , earliestShardIndex_(seqToShardIndex(earliestLedgerSeq())) , avgShardFileSz_(ledgersPerShard_ * kilobytes(192ull)) , openFinalLimit_( @@ -224,7 +218,6 @@ DatabaseShardImp::init() } updateStatus(lock); - setParent(parent_); init_ = true; } @@ -692,45 +685,31 @@ DatabaseShardImp::getCompleteShards() } void -DatabaseShardImp::onStop() +DatabaseShardImp::stop() { // Stop read threads in base before data members are destroyed - stopReadThreads(); - - std::lock_guard lock(mutex_); - - // Notify shards to stop - for (auto const& e : shards_) - e.second->stop(); -} - -void -DatabaseShardImp::onChildrenStopped() -{ + Database::stop(); std::vector> shards; { std::lock_guard lock(mutex_); - shards.reserve(shards_.size()); - for (auto const& e : shards_) - shards.push_back(e.second); + for (auto const& [_, shard] : shards_) + { + shards.push_back(shard); + shard->stop(); + } shards_.clear(); } + taskQueue_.stop(); // All shards should be expired at this point - for (auto const& e : shards) + for (auto const& wptr : shards) { - if (!e.expired()) + if (auto const shard{wptr.lock()}) { - std::string shardIndex; - if (auto const shard{e.lock()}; shard) - shardIndex = std::to_string(shard->index()); - - JLOG(j_.warn()) << " shard " << shardIndex << " unexpired"; + JLOG(j_.warn()) << " shard " << shard->index() << " unexpired"; } } - - stopped(); } void @@ -1303,10 +1282,10 @@ DatabaseShardImp::finalizeShard( bool writeSQLite, std::optional const& expectedHash) { - taskQueue_->addTask([this, - wptr = std::weak_ptr(shard), - writeSQLite, - expectedHash]() { + taskQueue_.addTask([this, + wptr = std::weak_ptr(shard), + writeSQLite, + expectedHash]() { if (isStopping()) return; @@ -2047,7 +2026,6 @@ DatabaseShardImp::iterateTransactionSQLsBack( std::unique_ptr make_ShardStore( Application& app, - Stoppable& parent, Scheduler& scheduler, int readThreads, beast::Journal j) @@ -2058,8 +2036,7 @@ make_ShardStore( if (section.empty()) return nullptr; - return std::make_unique( - app, parent, "ShardStore", scheduler, readThreads, j); + return std::make_unique(app, scheduler, readThreads, j); } } // namespace NodeStore diff --git a/src/ripple/nodestore/impl/DatabaseShardImp.h b/src/ripple/nodestore/impl/DatabaseShardImp.h index 3d5028c51e..e270229111 100644 --- a/src/ripple/nodestore/impl/DatabaseShardImp.h +++ b/src/ripple/nodestore/impl/DatabaseShardImp.h @@ -42,12 +42,15 @@ public: DatabaseShardImp( Application& app, - Stoppable& parent, - std::string const& name, Scheduler& scheduler, int readThreads, beast::Journal j); + ~DatabaseShardImp() + { + stop(); + } + [[nodiscard]] bool init() override; @@ -124,10 +127,7 @@ public: } void - onStop() override; - - void - onChildrenStopped() override; + stop() override; /** Import the application local node store @@ -204,7 +204,6 @@ private: }; Application& app_; - Stoppable& parent_; mutable std::mutex mutex_; bool init_{false}; @@ -212,7 +211,7 @@ private: std::unique_ptr ctx_; // Queue of background tasks to be performed - std::unique_ptr taskQueue_; + TaskQueue taskQueue_; // Shards held by this server std::map> shards_; diff --git a/src/ripple/nodestore/impl/ManagerImp.cpp b/src/ripple/nodestore/impl/ManagerImp.cpp index 4a823ea825..0a2e1712de 100644 --- a/src/ripple/nodestore/impl/ManagerImp.cpp +++ b/src/ripple/nodestore/impl/ManagerImp.cpp @@ -70,24 +70,16 @@ ManagerImp::make_Backend( std::unique_ptr ManagerImp::make_Database( - std::string const& name, std::size_t burstSize, Scheduler& scheduler, int readThreads, - Stoppable& parent, Section const& config, beast::Journal journal) { auto backend{make_Backend(config, burstSize, scheduler, journal)}; backend->open(); return std::make_unique( - name, - scheduler, - readThreads, - parent, - std::move(backend), - config, - journal); + scheduler, readThreads, std::move(backend), config, journal); } void diff --git a/src/ripple/nodestore/impl/ManagerImp.h b/src/ripple/nodestore/impl/ManagerImp.h index b062d677a1..42f8584ef0 100644 --- a/src/ripple/nodestore/impl/ManagerImp.h +++ b/src/ripple/nodestore/impl/ManagerImp.h @@ -61,11 +61,9 @@ public: std::unique_ptr make_Database( - std::string const& name, std::size_t burstSize, Scheduler& scheduler, int readThreads, - Stoppable& parent, Section const& config, beast::Journal journal) override; }; diff --git a/src/ripple/nodestore/impl/TaskQueue.cpp b/src/ripple/nodestore/impl/TaskQueue.cpp index 000b664b0b..7b917fdd77 100644 --- a/src/ripple/nodestore/impl/TaskQueue.cpp +++ b/src/ripple/nodestore/impl/TaskQueue.cpp @@ -24,25 +24,23 @@ namespace ripple { namespace NodeStore { -TaskQueue::TaskQueue(Stoppable& parent) - : Stoppable("TaskQueue", parent) - , workers_(*this, nullptr, "Shard store taskQueue", 1) +TaskQueue::TaskQueue() : workers_(*this, nullptr, "Shard store taskQueue", 1) { } void -TaskQueue::onStop() +TaskQueue::stop() { - workers_.pauseAllThreadsAndWait(); - stopped(); + workers_.stop(); } void TaskQueue::addTask(std::function task) { - std::lock_guard lock{mutex_}; - - tasks_.emplace(std::move(task)); + { + std::lock_guard lock{mutex_}; + tasks_.emplace(std::move(task)); + } workers_.addTask(); } diff --git a/src/ripple/nodestore/impl/TaskQueue.h b/src/ripple/nodestore/impl/TaskQueue.h index 9f2121a450..77c5ff95b8 100644 --- a/src/ripple/nodestore/impl/TaskQueue.h +++ b/src/ripple/nodestore/impl/TaskQueue.h @@ -20,7 +20,6 @@ #ifndef RIPPLE_NODESTORE_TASKQUEUE_H_INCLUDED #define RIPPLE_NODESTORE_TASKQUEUE_H_INCLUDED -#include #include #include @@ -29,13 +28,13 @@ namespace ripple { namespace NodeStore { -class TaskQueue : public Stoppable, private Workers::Callback +class TaskQueue : private Workers::Callback { public: - explicit TaskQueue(Stoppable& parent); + TaskQueue(); void - onStop() override; + stop(); /** Adds a task to the queue diff --git a/src/ripple/overlay/Overlay.h b/src/ripple/overlay/Overlay.h index b8675f02ac..35528d6270 100644 --- a/src/ripple/overlay/Overlay.h +++ b/src/ripple/overlay/Overlay.h @@ -21,7 +21,6 @@ #define RIPPLE_OVERLAY_OVERLAY_H_INCLUDED #include -#include #include #include #include @@ -49,7 +48,7 @@ class context; namespace ripple { /** Manages the set of connected peers. */ -class Overlay : public Stoppable, public beast::PropertyStream::Source +class Overlay : public beast::PropertyStream::Source { protected: using socket_type = boost::beast::tcp_stream; @@ -57,10 +56,8 @@ protected: // VFALCO NOTE The requirement of this constructor is an // unfortunate problem with the API for - // Stoppable and PropertyStream - // - Overlay(Stoppable& parent) - : Stoppable("Overlay", parent), beast::PropertyStream::Source("peers") + // PropertyStream + Overlay() : beast::PropertyStream::Source("peers") { } @@ -83,6 +80,16 @@ public: virtual ~Overlay() = default; + virtual void + start() + { + } + + virtual void + stop() + { + } + /** Conditionally accept an incoming HTTP request. */ virtual Handoff onHandoff( diff --git a/src/ripple/overlay/impl/OverlayImpl.cpp b/src/ripple/overlay/impl/OverlayImpl.cpp index aecb516b17..e5bc1c8a92 100644 --- a/src/ripple/overlay/impl/OverlayImpl.cpp +++ b/src/ripple/overlay/impl/OverlayImpl.cpp @@ -73,14 +73,16 @@ OverlayImpl::Timer::Timer(OverlayImpl& overlay) void OverlayImpl::Timer::stop() { - error_code ec; - timer_.cancel(ec); + // This method is only ever called from the same strand that calls + // Timer::on_timer, ensuring they never execute concurrently. + stopping_ = true; + timer_.cancel(); } void -OverlayImpl::Timer::run() +OverlayImpl::Timer::async_wait() { - timer_.expires_from_now(std::chrono::seconds(1)); + timer_.expires_after(std::chrono::seconds(1)); timer_.async_wait(overlay_.strand_.wrap(std::bind( &Timer::on_timer, shared_from_this(), std::placeholders::_1))); } @@ -88,7 +90,7 @@ OverlayImpl::Timer::run() void OverlayImpl::Timer::on_timer(error_code ec) { - if (ec || overlay_.isStopping()) + if (ec || stopping_) { if (ec && ec != boost::asio::error::operation_aborted) { @@ -104,9 +106,7 @@ OverlayImpl::Timer::on_timer(error_code ec) if ((++overlay_.timer_count_ % Tuning::checkIdlePeers) == 0) overlay_.deleteIdlePeers(); - timer_.expires_from_now(std::chrono::seconds(1)); - timer_.async_wait(overlay_.strand_.wrap(std::bind( - &Timer::on_timer, shared_from_this(), std::placeholders::_1))); + async_wait(); } //------------------------------------------------------------------------------ @@ -114,15 +114,13 @@ OverlayImpl::Timer::on_timer(error_code ec) OverlayImpl::OverlayImpl( Application& app, Setup const& setup, - Stoppable& parent, ServerHandler& serverHandler, Resource::Manager& resourceManager, Resolver& resolver, boost::asio::io_service& io_service, BasicConfig const& config, beast::insight::Collector::ptr const& collector) - : Overlay(parent) - , app_(app) + : app_(app) , io_service_(io_service) , work_(std::in_place, std::ref(io_service_)) , strand_(io_service_) @@ -131,7 +129,6 @@ OverlayImpl::OverlayImpl( , serverHandler_(serverHandler) , m_resourceManager(resourceManager) , m_peerFinder(PeerFinder::make_Manager( - *this, io_service, stopwatch(), app_.journal("PeerFinder"), @@ -159,19 +156,6 @@ OverlayImpl::OverlayImpl( beast::PropertyStream::Source::add(m_peerFinder.get()); } -OverlayImpl::~OverlayImpl() -{ - stop(); - - // Block until dependent objects have been destroyed. - // This is just to catch improper use of the Stoppable API. - // - std::unique_lock lock(mutex_); - cond_.wait(lock, [this] { return list_.empty(); }); -} - -//------------------------------------------------------------------------------ - Handoff OverlayImpl::onHandoff( std::unique_ptr&& stream_ptr, @@ -480,22 +464,8 @@ OverlayImpl::remove(std::shared_ptr const& slot) m_peers.erase(iter); } -//------------------------------------------------------------------------------ -// -// Stoppable -// -//------------------------------------------------------------------------------ - -// Caller must hold the mutex void -OverlayImpl::checkStopped() -{ - if (isStopping() && areChildrenStopped() && list_.empty()) - stopped(); -} - -void -OverlayImpl::onPrepare() +OverlayImpl::start() { PeerFinder::Config config = PeerFinder::Config::makeConfig( app_.config(), @@ -504,6 +474,7 @@ OverlayImpl::onPrepare() setup_.ipLimit); m_peerFinder->setConfig(config); + m_peerFinder->start(); // Populate our boot cache: if there are no entries in [ips] then we use // the entries in [ips_fixed]. @@ -567,29 +538,22 @@ OverlayImpl::onPrepare() m_peerFinder->addFixedPeer(name, ips); }); } -} - -void -OverlayImpl::onStart() -{ auto const timer = std::make_shared(*this); std::lock_guard lock(mutex_); list_.emplace(timer.get(), timer); timer_ = timer; - timer->run(); + timer->async_wait(); } void -OverlayImpl::onStop() +OverlayImpl::stop() { - strand_.dispatch(std::bind(&OverlayImpl::stop, this)); -} - -void -OverlayImpl::onChildrenStopped() -{ - std::lock_guard lock(mutex_); - checkStopped(); + strand_.dispatch(std::bind(&OverlayImpl::stopChildren, this)); + { + std::unique_lock lock(mutex_); + cond_.wait(lock, [this] { return list_.empty(); }); + } + m_peerFinder->stop(); } //------------------------------------------------------------------------------ @@ -1299,11 +1263,11 @@ OverlayImpl::remove(Child& child) std::lock_guard lock(mutex_); list_.erase(&child); if (list_.empty()) - checkStopped(); + cond_.notify_all(); } void -OverlayImpl::stop() +OverlayImpl::stopChildren() { // Calling list_[].second->stop() may cause list_ to be modified // (OverlayImpl::remove() may be called on this same thread). So @@ -1561,7 +1525,6 @@ std::unique_ptr make_Overlay( Application& app, Overlay::Setup const& setup, - Stoppable& parent, ServerHandler& serverHandler, Resource::Manager& resourceManager, Resolver& resolver, @@ -1572,7 +1535,6 @@ make_Overlay( return std::make_unique( app, setup, - parent, serverHandler, resourceManager, resolver, diff --git a/src/ripple/overlay/impl/OverlayImpl.h b/src/ripple/overlay/impl/OverlayImpl.h index d670a27317..f58148d578 100644 --- a/src/ripple/overlay/impl/OverlayImpl.h +++ b/src/ripple/overlay/impl/OverlayImpl.h @@ -81,6 +81,7 @@ private: struct Timer : Child, std::enable_shared_from_this { boost::asio::basic_waitable_timer timer_; + bool stopping_{false}; explicit Timer(OverlayImpl& overlay); @@ -88,7 +89,7 @@ private: stop() override; void - run(); + async_wait(); void on_timer(error_code ec); @@ -141,7 +142,6 @@ public: OverlayImpl( Application& app, Setup const& setup, - Stoppable& parent, ServerHandler& serverHandler, Resource::Manager& resourceManager, Resolver& resolver, @@ -149,12 +149,16 @@ public: BasicConfig const& config, beast::insight::Collector::ptr const& collector); - ~OverlayImpl(); - OverlayImpl(OverlayImpl const&) = delete; OverlayImpl& operator=(OverlayImpl const&) = delete; + void + start() override; + + void + stop() override; + PeerFinder::Manager& peerFinder() { @@ -500,25 +504,6 @@ private: //-------------------------------------------------------------------------- - // - // Stoppable - // - - void - checkStopped(); - - void - onPrepare() override; - - void - onStart() override; - - void - onStop() override; - - void - onChildrenStopped() override; - // // PropertyStream // @@ -532,7 +517,7 @@ private: remove(Child& child); void - stop(); + stopChildren(); void autoConnect(); diff --git a/src/ripple/overlay/make_Overlay.h b/src/ripple/overlay/make_Overlay.h index 5a6b647b7a..9a63f5a46a 100644 --- a/src/ripple/overlay/make_Overlay.h +++ b/src/ripple/overlay/make_Overlay.h @@ -21,7 +21,6 @@ #define RIPPLE_OVERLAY_MAKE_OVERLAY_H_INCLUDED #include -#include #include #include #include @@ -38,7 +37,6 @@ std::unique_ptr make_Overlay( Application& app, Overlay::Setup const& setup, - Stoppable& parent, ServerHandler& serverHandler, Resource::Manager& resourceManager, Resolver& resolver, diff --git a/src/ripple/peerfinder/PeerfinderManager.h b/src/ripple/peerfinder/PeerfinderManager.h index bcca3c919c..61ce3df143 100644 --- a/src/ripple/peerfinder/PeerfinderManager.h +++ b/src/ripple/peerfinder/PeerfinderManager.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -133,10 +132,10 @@ using Endpoints = std::vector; enum class Result { duplicate, full, success }; /** Maintains a set of IP addresses used for getting into the network. */ -class Manager : public Stoppable, public beast::PropertyStream::Source +class Manager : public beast::PropertyStream::Source { protected: - explicit Manager(Stoppable& parent); + Manager() noexcept; public: /** Destroy the object. @@ -154,6 +153,14 @@ public: virtual void setConfig(Config const& config) = 0; + /** Transition to the started state, synchronously. */ + virtual void + start() = 0; + + /** Transition to the stopped state, synchronously. */ + virtual void + stop() = 0; + /** Returns the configuration for the manager. */ virtual Config config() = 0; diff --git a/src/ripple/peerfinder/impl/PeerfinderManager.cpp b/src/ripple/peerfinder/impl/PeerfinderManager.cpp index ed41c86520..e3743c047e 100644 --- a/src/ripple/peerfinder/impl/PeerfinderManager.cpp +++ b/src/ripple/peerfinder/impl/PeerfinderManager.cpp @@ -46,13 +46,12 @@ public: //-------------------------------------------------------------------------- ManagerImp( - Stoppable& stoppable, boost::asio::io_service& io_service, clock_type& clock, beast::Journal journal, BasicConfig const& config, beast::insight::Collector::ptr const& collector) - : Manager(stoppable) + : Manager() , io_service_(io_service) , work_(std::in_place, std::ref(io_service_)) , m_clock(clock) @@ -67,11 +66,11 @@ public: ~ManagerImp() override { - close(); + stop(); } void - close() + stop() override { if (work_) { @@ -213,26 +212,13 @@ public: return m_logic.buildEndpointsForPeers(); } - //-------------------------------------------------------------------------- - // - // Stoppable - // - //-------------------------------------------------------------------------- - void - onPrepare() override + start() override { m_store.open(m_config); m_logic.load(); } - void - onStop() override - { - close(); - stopped(); - } - //-------------------------------------------------------------------------- // // PropertyStream @@ -279,15 +265,12 @@ private: //------------------------------------------------------------------------------ -Manager::Manager(Stoppable& parent) - : Stoppable("PeerFinder", parent) - , beast::PropertyStream::Source("peerfinder") +Manager::Manager() noexcept : beast::PropertyStream::Source("peerfinder") { } std::unique_ptr make_Manager( - Stoppable& parent, boost::asio::io_service& io_service, clock_type& clock, beast::Journal journal, @@ -295,7 +278,7 @@ make_Manager( beast::insight::Collector::ptr const& collector) { return std::make_unique( - parent, io_service, clock, journal, config, collector); + io_service, clock, journal, config, collector); } } // namespace PeerFinder diff --git a/src/ripple/peerfinder/make_Manager.h b/src/ripple/peerfinder/make_Manager.h index 2520fa360d..932fccb9ab 100644 --- a/src/ripple/peerfinder/make_Manager.h +++ b/src/ripple/peerfinder/make_Manager.h @@ -30,7 +30,6 @@ namespace PeerFinder { /** Create a new Manager. */ std::unique_ptr make_Manager( - Stoppable& parent, boost::asio::io_service& io_service, clock_type& clock, beast::Journal journal, diff --git a/src/ripple/rpc/ServerHandler.h b/src/ripple/rpc/ServerHandler.h index 1a9d34b650..54cccdff64 100644 --- a/src/ripple/rpc/ServerHandler.h +++ b/src/ripple/rpc/ServerHandler.h @@ -24,7 +24,6 @@ #include #include #include -#include #include #include #include @@ -43,7 +42,6 @@ setup_ServerHandler(Config const& c, std::ostream&& log); std::unique_ptr make_ServerHandler( Application& app, - Stoppable& parent, boost::asio::io_service&, JobQueue&, NetworkOPs&, diff --git a/src/ripple/rpc/ShardArchiveHandler.h b/src/ripple/rpc/ShardArchiveHandler.h index 500dee20f4..221624885c 100644 --- a/src/ripple/rpc/ShardArchiveHandler.h +++ b/src/ripple/rpc/ShardArchiveHandler.h @@ -37,7 +37,7 @@ class ShardArchiveHandler_test; namespace RPC { /** Handles the download and import of one or more shard archives. */ -class ShardArchiveHandler : public Stoppable +class ShardArchiveHandler { public: using TimerOpCounter = @@ -48,15 +48,15 @@ public: getDownloadDirectory(Config const& config); static std::unique_ptr - makeShardArchiveHandler(Application& app, Stoppable& parent); + makeShardArchiveHandler(Application& app); // Create a ShardArchiveHandler only if // the state database is present, indicating // that recovery is needed. static std::unique_ptr - tryMakeRecoveryHandler(Application& app, Stoppable& parent); + tryMakeRecoveryHandler(Application& app); - ShardArchiveHandler(Application& app, Stoppable& parent); + ShardArchiveHandler(Application& app); virtual ~ShardArchiveHandler() = default; @@ -70,6 +70,9 @@ public: bool start(); + void + stop(); + void release(); @@ -84,9 +87,6 @@ private: [[nodiscard]] bool initFromDB(std::lock_guard const&); - void - onStop() override; - /** Add an archive to be downloaded and imported. @param shardIndex the index of the shard to be imported. @param url the location of the archive. @@ -131,6 +131,7 @@ private: // destroying sqlDB_. ///////////////////////////////////////////////// std::mutex mutable m_; + std::atomic_bool stopping_{false}; std::shared_ptr downloader_; std::map archives_; bool process_; @@ -162,7 +163,7 @@ private: class RecoveryHandler : public ShardArchiveHandler { public: - RecoveryHandler(Application& app, Stoppable& parent); + RecoveryHandler(Application& app); }; } // namespace RPC diff --git a/src/ripple/rpc/handlers/LedgerCleanerHandler.cpp b/src/ripple/rpc/handlers/LedgerCleanerHandler.cpp index 00e142c4d6..5b76f3f8b9 100644 --- a/src/ripple/rpc/handlers/LedgerCleanerHandler.cpp +++ b/src/ripple/rpc/handlers/LedgerCleanerHandler.cpp @@ -17,7 +17,7 @@ */ //============================================================================== -#include +#include #include #include #include @@ -28,7 +28,7 @@ namespace ripple { Json::Value doLedgerCleaner(RPC::JsonContext& context) { - context.app.getLedgerMaster().doLedgerCleaner(context.params); + context.app.getLedgerCleaner().clean(context.params); return RPC::makeObjectValue("Cleaner configured"); } diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp index d64d58bcb9..9217188c72 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.cpp +++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp @@ -48,6 +48,7 @@ #include #include #include +#include #include namespace ripple { @@ -102,14 +103,12 @@ authorized(Port const& port, std::map const& h) ServerHandlerImp::ServerHandlerImp( Application& app, - Stoppable& parent, boost::asio::io_service& io_service, JobQueue& jobQueue, NetworkOPs& networkOPs, Resource::Manager& resourceManager, CollectorManager& cm) - : Stoppable("ServerHandler", parent) - , app_(app) + : app_(app) , m_resourceManager(resourceManager) , m_journal(app_.journal("Server")) , m_networkOPs(networkOPs) @@ -137,9 +136,13 @@ ServerHandlerImp::setup(Setup const& setup, beast::Journal journal) //------------------------------------------------------------------------------ void -ServerHandlerImp::onStop() +ServerHandlerImp::stop() { m_server->close(); + { + std::unique_lock lock(mutex_); + condition_.wait(lock, [this] { return stopped_; }); + } } //------------------------------------------------------------------------------ @@ -149,14 +152,17 @@ ServerHandlerImp::onAccept( Session& session, boost::asio::ip::tcp::endpoint endpoint) { - std::lock_guard lock(countlock_); + auto const& port = session.port(); - auto const c = ++count_[session.port()]; + auto const c = [this, &port]() { + std::lock_guard lock(mutex_); + return ++count_[port]; + }(); - if (session.port().limit && c >= session.port().limit) + if (port.limit && c >= port.limit) { JLOG(m_journal.trace()) - << session.port().name << " is full; dropping " << endpoint; + << port.name << " is full; dropping " << endpoint; return false; } @@ -356,14 +362,16 @@ ServerHandlerImp::onWSMessage( void ServerHandlerImp::onClose(Session& session, boost::system::error_code const&) { - std::lock_guard lock(countlock_); + std::lock_guard lock(mutex_); --count_[session.port()]; } void ServerHandlerImp::onStopped(Server&) { - stopped(); + std::lock_guard lock(mutex_); + stopped_ = true; + condition_.notify_one(); } //------------------------------------------------------------------------------ @@ -1167,7 +1175,6 @@ setup_ServerHandler(Config const& config, std::ostream&& log) std::unique_ptr make_ServerHandler( Application& app, - Stoppable& parent, boost::asio::io_service& io_service, JobQueue& jobQueue, NetworkOPs& networkOPs, @@ -1175,7 +1182,7 @@ make_ServerHandler( CollectorManager& cm) { return std::make_unique( - app, parent, io_service, jobQueue, networkOPs, resourceManager, cm); + app, io_service, jobQueue, networkOPs, resourceManager, cm); } } // namespace ripple diff --git a/src/ripple/rpc/impl/ServerHandlerImp.h b/src/ripple/rpc/impl/ServerHandlerImp.h index 49301cb07c..7c0bf9c9ae 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.h +++ b/src/ripple/rpc/impl/ServerHandlerImp.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -43,7 +44,7 @@ operator<(Port const& lhs, Port const& rhs) return lhs.name < rhs.name; } -class ServerHandlerImp : public Stoppable +class ServerHandlerImp { public: struct Setup @@ -98,13 +99,14 @@ private: beast::insight::Counter rpc_requests_; beast::insight::Event rpc_size_; beast::insight::Event rpc_time_; - std::mutex countlock_; + std::mutex mutex_; + std::condition_variable condition_; + bool stopped_{false}; std::map, int> count_; public: ServerHandlerImp( Application& app, - Stoppable& parent, boost::asio::io_service& io_service, JobQueue& jobQueue, NetworkOPs& networkOPs, @@ -124,12 +126,8 @@ public: return setup_; } - // - // Stoppable - // - void - onStop() override; + stop(); // // Handler diff --git a/src/ripple/rpc/impl/ShardArchiveHandler.cpp b/src/ripple/rpc/impl/ShardArchiveHandler.cpp index 83928032c4..7ad8bcbbad 100644 --- a/src/ripple/rpc/impl/ShardArchiveHandler.cpp +++ b/src/ripple/rpc/impl/ShardArchiveHandler.cpp @@ -46,15 +46,13 @@ ShardArchiveHandler::getDownloadDirectory(Config const& config) } std::unique_ptr -ShardArchiveHandler::makeShardArchiveHandler( - Application& app, - Stoppable& parent) +ShardArchiveHandler::makeShardArchiveHandler(Application& app) { - return std::make_unique(app, parent); + return std::make_unique(app); } std::unique_ptr -ShardArchiveHandler::tryMakeRecoveryHandler(Application& app, Stoppable& parent) +ShardArchiveHandler::tryMakeRecoveryHandler(Application& app) { auto const downloadDir(getDownloadDirectory(app.config())); @@ -63,15 +61,14 @@ ShardArchiveHandler::tryMakeRecoveryHandler(Application& app, Stoppable& parent) if (exists(downloadDir / stateDBName) && is_regular_file(downloadDir / stateDBName)) { - return std::make_unique(app, parent); + return std::make_unique(app); } return nullptr; } -ShardArchiveHandler::ShardArchiveHandler(Application& app, Stoppable& parent) - : Stoppable("ShardArchiveHandler", parent) - , process_(false) +ShardArchiveHandler::ShardArchiveHandler(Application& app) + : process_(false) , app_(app) , j_(app.journal("ShardArchiveHandler")) , downloadDir_(getDownloadDirectory(app.config())) @@ -176,14 +173,15 @@ ShardArchiveHandler::initFromDB(std::lock_guard const& lock) } void -ShardArchiveHandler::onStop() +ShardArchiveHandler::stop() { + stopping_ = true; { std::lock_guard lock(m_); if (downloader_) { - downloader_->onStop(); + downloader_->stop(); downloader_.reset(); } @@ -195,8 +193,6 @@ ShardArchiveHandler::onStop() timerCounter_.join( "ShardArchiveHandler", std::chrono::milliseconds(2000), j_); - - stopped(); } bool @@ -297,7 +293,7 @@ ShardArchiveHandler::release() bool ShardArchiveHandler::next(std::lock_guard const& l) { - if (isStopping()) + if (stopping_) return false; if (archives_.empty()) @@ -394,7 +390,7 @@ ShardArchiveHandler::next(std::lock_guard const& l) void ShardArchiveHandler::complete(path dstPath) { - if (isStopping()) + if (stopping_) return; { @@ -422,7 +418,7 @@ ShardArchiveHandler::complete(path dstPath) // Make lambdas mutable captured vars can be moved from auto wrapper = jobCounter_.wrap([=, dstPath = std::move(dstPath)](Job&) mutable { - if (isStopping()) + if (stopping_) return; // If not synced then defer and retry @@ -459,7 +455,7 @@ ShardArchiveHandler::complete(path dstPath) if (!wrapper) { - if (isStopping()) + if (stopping_) return; JLOG(j_.error()) << "failed to wrap closure for process()"; @@ -565,7 +561,7 @@ ShardArchiveHandler::onClosureFailed( std::string const& errorMsg, std::lock_guard const& lock) { - if (isStopping()) + if (stopping_) return false; JLOG(j_.error()) << errorMsg; @@ -580,8 +576,7 @@ ShardArchiveHandler::removeAndProceed(std::lock_guard const& lock) return next(lock); } -RecoveryHandler::RecoveryHandler(Application& app, Stoppable& parent) - : ShardArchiveHandler(app, parent) +RecoveryHandler::RecoveryHandler(Application& app) : ShardArchiveHandler(app) { } diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index a1a59d3f01..4297c78d1f 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -169,7 +169,7 @@ public: } virtual void - onStop() override + stop() override { } @@ -564,7 +564,6 @@ public: inboundBhvr) , serverMsgHandler(server.app, server.app.getLedgerReplayer()) , clientMsgHandler(env.app(), replayer) - , stopableParent("replayerStopParent") , replayer( env.app(), inboundLedgers, @@ -572,8 +571,7 @@ public: clientMsgHandler, serverMsgHandler, behavior, - peerFeature), - stopableParent) + peerFeature)) { } @@ -800,7 +798,6 @@ public: MagicInboundLedgers inboundLedgers; LedgerReplayMsgHandler serverMsgHandler; LedgerReplayMsgHandler clientMsgHandler; - RootStoppable stopableParent; LedgerReplayer replayer; }; @@ -848,7 +845,7 @@ struct NetworkOfTwo * -- replay a range of ledgers and fallback to InboundLedgers because * peers do not support ProtocolFeature::LedgerReplay * -- replay a range of ledgers and the network drops or repeats messages - * -- call onStop() and the tasks and subtasks are removed + * -- call stop() and the tasks and subtasks are removed * -- process a bad skip list * -- process a bad ledger delta * -- replay ledger ranges with different overlaps @@ -1229,9 +1226,9 @@ struct LedgerReplayer_test : public beast::unit_test::suite } void - testOnStop() + testStop() { - testcase("onStop before timeout"); + testcase("stop before timeout"); int totalReplay = 3; NetworkOfTwo net( *this, @@ -1253,9 +1250,8 @@ struct LedgerReplayer_test : public beast::unit_test::suite TaskStatus::NotDone, deltaStatuses)); - // onStop BEAST_EXPECT(net.client.countsAsExpected(1, 1, 0)); - net.client.replayer.onStop(); + net.client.replayer.stop(); BEAST_EXPECT(net.client.countsAsExpected(0, 0, 0)); } @@ -1441,7 +1437,7 @@ struct LedgerReplayer_test : public beast::unit_test::suite testPeerSetBehavior(PeerSetBehavior::Good); testPeerSetBehavior(PeerSetBehavior::Drop50); testPeerSetBehavior(PeerSetBehavior::Repeat); - testOnStop(); + testStop(); testSkipListBadReply(); testLedgerDeltaBadReply(); testLedgerReplayOverlap(); diff --git a/src/test/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index e259fc7b7c..db0cf88510 100644 --- a/src/test/basics/PerfLog_test.cpp +++ b/src/test/basics/PerfLog_test.cpp @@ -20,11 +20,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -45,145 +47,101 @@ class PerfLog_test : public beast::unit_test::suite test::jtx::Env env_{*this}; beast::Journal j_{env_.app().journal("PerfLog_test")}; - // A PerfLog needs a Parent that is a Stoppable and a function to - // call if it wants to shutdown the system. This class provides both. - struct PerfLogParent : public RootStoppable + struct Fixture { - bool stopSignaled{false}; beast::Journal j_; + bool stopSignaled{false}; - explicit PerfLogParent(beast::Journal const& j) - : RootStoppable("testRootStoppable"), j_(j) + Fixture(beast::Journal const& j) : j_(j) { } - ~PerfLogParent() override + ~Fixture() { - doStop(); - cleanupPerfLogDir(); + using namespace boost::filesystem; + + auto const dir{logDir()}; + auto const file{logFile()}; + if (exists(file)) + remove(file); + + if (!exists(dir) || !is_directory(dir) || !is_empty(dir)) + { + return; + } + remove(dir); } - // Benign replacement for Application::signalStop(). void signalStop() { stopSignaled = true; } - private: - // Interfaces for RootStoppable. Made private to encourage the use - // of doStart() and doStop(). - void - onPrepare() override - { - } - - void - onStart() override - { - } - - void - onStop() override - { - if (areChildrenStopped()) - stopped(); - } - - void - onChildrenStopped() override - { - onStop(); - } - - public: - // Test interfaces to start and stop the PerfLog - void - doStart() - { - start(); - } - - void - doStop() - { - if (started()) - stop(j_); - } - - // Interfaces for PerfLog file management - static path - getPerfLogPath() + path + logDir() const { using namespace boost::filesystem; return temp_directory_path() / "perf_log_test_dir"; } - static path - getPerfLogFileName() + path + logFile() const { - return {"perf_log.txt"}; + return logDir() / "perf_log.txt"; } - static std::chrono::milliseconds - getLogInterval() + std::chrono::milliseconds + logInterval() const { return std::chrono::milliseconds{10}; } - static perf::PerfLog::Setup - getSetup(WithFile withFile) + std::unique_ptr + perfLog(WithFile withFile) { - return perf::PerfLog::Setup{ - withFile == WithFile::no - ? "" - : getPerfLogPath() / getPerfLogFileName(), - getLogInterval()}; + perf::PerfLog::Setup const setup{ + withFile == WithFile::no ? "" : logFile(), logInterval()}; + return perf::make_PerfLog( + setup, j_, [this]() { return signalStop(); }); } - static void - cleanupPerfLogDir() + // Block until the log file has grown in size, indicating that the + // PerfLog has written new values to the file and _should_ have the + // latest update. + void + wait() const { using namespace boost::filesystem; - auto const perfLogPath{getPerfLogPath()}; - auto const fullPath = perfLogPath / getPerfLogFileName(); - if (exists(fullPath)) - remove(fullPath); - - if (!exists(perfLogPath) || !is_directory(perfLogPath) || - !is_empty(perfLogPath)) - { + auto const path = logFile(); + if (!exists(path)) return; - } - remove(perfLogPath); + + // We wait for the file to change size twice. The first file size + // change may have been in process while we arrived. + std::uintmax_t const firstSize{file_size(path)}; + std::uintmax_t secondSize{firstSize}; + do + { + std::this_thread::sleep_for(logInterval()); + secondSize = file_size(path); + } while (firstSize >= secondSize); + + do + { + std::this_thread::sleep_for(logInterval()); + } while (secondSize >= file_size(path)); } }; - //------------------------------------------------------------------------------ - - // Convenience function to return a PerfLog - std::unique_ptr - getPerfLog(PerfLogParent& parent, WithFile withFile) - { - return perf::make_PerfLog( - parent.getSetup(withFile), parent, j_, [&parent]() { - return parent.signalStop(); - }); - } - - //------------------------------------------------------------------------------ - - // Convenience function to return a uint64 given a Json::Value containing - // a string. + // Return a uint64 from a JSON string. static std::uint64_t jsonToUint64(Json::Value const& jsonUintAsString) { return std::stoull(jsonUintAsString.asString()); } - //------------------------------------------------------------------------------ - // The PerfLog's current state is easier to sort by duration if the // duration is converted from string to integer. The following struct // is a way to think about the converted entry. @@ -226,70 +184,35 @@ class PerfLog_test : public beast::unit_test::suite return currents; } - //------------------------------------------------------------------------------ - - // Helper function that checks the size of the PerfLog file and then - // returns when the file gets bigger. This indicates that the PerfLog - // has written new values to the file and _should_ have the latest - // update. - static void - waitForFileUpdate(PerfLogParent const& parent) - { - using namespace boost::filesystem; - - auto const path = parent.getPerfLogPath() / parent.getPerfLogFileName(); - if (!exists(path)) - return; - - // We wait for the file to change size twice. The first file size - // change may have been in process while we arrived. - std::uintmax_t const firstSize{file_size(path)}; - std::uintmax_t secondSize{firstSize}; - do - { - std::this_thread::sleep_for(parent.getLogInterval()); - secondSize = file_size(path); - } while (firstSize >= secondSize); - - do - { - std::this_thread::sleep_for(parent.getLogInterval()); - } while (secondSize >= file_size(path)); - } - - //------------------------------------------------------------------------------ - public: void testFileCreation() { using namespace boost::filesystem; - auto const perfLogPath{PerfLogParent::getPerfLogPath()}; - auto const fullPath = perfLogPath / PerfLogParent::getPerfLogFileName(); { // Verify a PerfLog creates its file when constructed. - PerfLogParent parent{j_}; - BEAST_EXPECT(!exists(perfLogPath)); + Fixture fixture{j_}; + BEAST_EXPECT(!exists(fixture.logFile())); - auto perfLog{getPerfLog(parent, WithFile::yes)}; + auto perfLog{fixture.perfLog(WithFile::yes)}; - BEAST_EXPECT(parent.stopSignaled == false); - BEAST_EXPECT(exists(perfLogPath)); + BEAST_EXPECT(fixture.stopSignaled == false); + BEAST_EXPECT(exists(fixture.logFile())); } { // Create a file where PerfLog wants to put its directory. // Make sure that PerfLog tries to shutdown the server since it // can't open its file. - PerfLogParent parent{j_}; - if (!BEAST_EXPECT(!exists(perfLogPath))) + Fixture fixture{j_}; + if (!BEAST_EXPECT(!exists(fixture.logDir()))) return; { // Make a file that prevents PerfLog from creating its file. std::ofstream nastyFile; nastyFile.open( - perfLogPath.c_str(), std::ios::out | std::ios::app); + fixture.logDir().c_str(), std::ios::out | std::ios::app); if (!BEAST_EXPECT(nastyFile)) return; nastyFile.close(); @@ -297,32 +220,32 @@ public: // Now construct a PerfLog. The PerfLog should attempt to shut // down the server because it can't open its file. - BEAST_EXPECT(parent.stopSignaled == false); - auto perfLog{getPerfLog(parent, WithFile::yes)}; - BEAST_EXPECT(parent.stopSignaled == true); + BEAST_EXPECT(fixture.stopSignaled == false); + auto perfLog{fixture.perfLog(WithFile::yes)}; + BEAST_EXPECT(fixture.stopSignaled == true); // Start PerfLog and wait long enough for PerfLog::report() // to not be able to write to its file. That should cause no // problems. - parent.doStart(); - std::this_thread::sleep_for(parent.getLogInterval() * 10); - parent.doStop(); + perfLog->start(); + std::this_thread::sleep_for(fixture.logInterval() * 10); + perfLog->stop(); // Remove the file. - remove(perfLogPath); + remove(fixture.logDir()); } { // Put a write protected file where PerfLog wants to write its // file. Make sure that PerfLog tries to shutdown the server // since it can't open its file. - PerfLogParent parent{j_}; - if (!BEAST_EXPECT(!exists(perfLogPath))) + Fixture fixture{j_}; + if (!BEAST_EXPECT(!exists(fixture.logDir()))) return; // Construct and write protect a file to prevent PerfLog // from creating its file. boost::system::error_code ec; - boost::filesystem::create_directories(perfLogPath, ec); + boost::filesystem::create_directories(fixture.logDir(), ec); if (!BEAST_EXPECT(!ec)) return; @@ -331,17 +254,17 @@ public: .is_open(); }; - if (!BEAST_EXPECT(fileWriteable(fullPath))) + if (!BEAST_EXPECT(fileWriteable(fixture.logFile()))) return; boost::filesystem::permissions( - fullPath, + fixture.logFile(), perms::remove_perms | perms::owner_write | perms::others_write | perms::group_write); // If the test is running as root, then the write protect may have // no effect. Make sure write protect worked before proceeding. - if (fileWriteable(fullPath)) + if (fileWriteable(fixture.logFile())) { log << "Unable to write protect file. Test skipped." << std::endl; @@ -350,20 +273,20 @@ public: // Now construct a PerfLog. The PerfLog should attempt to shut // down the server because it can't open its file. - BEAST_EXPECT(parent.stopSignaled == false); - auto perfLog{getPerfLog(parent, WithFile::yes)}; - BEAST_EXPECT(parent.stopSignaled == true); + BEAST_EXPECT(fixture.stopSignaled == false); + auto perfLog{fixture.perfLog(WithFile::yes)}; + BEAST_EXPECT(fixture.stopSignaled == true); // Start PerfLog and wait long enough for PerfLog::report() // to not be able to write to its file. That should cause no // problems. - parent.doStart(); - std::this_thread::sleep_for(parent.getLogInterval() * 10); - parent.doStop(); + perfLog->start(); + std::this_thread::sleep_for(fixture.logInterval() * 10); + perfLog->stop(); // Fix file permissions so the file can be cleaned up. boost::filesystem::permissions( - fullPath, + fixture.logFile(), perms::add_perms | perms::owner_write | perms::others_write | perms::group_write); } @@ -374,9 +297,9 @@ public: { // Exercise the rpc interfaces of PerfLog. // Start up the PerfLog that we'll use for testing. - PerfLogParent parent{j_}; - auto perfLog{getPerfLog(parent, withFile)}; - parent.doStart(); + Fixture fixture{j_}; + auto perfLog{fixture.perfLog(withFile)}; + perfLog->start(); // Get the all the labels we can use for RPC interfaces without // causing an assert. @@ -534,13 +457,12 @@ public: validateFinalCurrent(perfLog->currentJson()); // Give the PerfLog enough time to flush it's state to the file. - waitForFileUpdate(parent); + fixture.wait(); // Politely stop the PerfLog. - parent.doStop(); + perfLog->stop(); - auto const fullPath = - parent.getPerfLogPath() / parent.getPerfLogFileName(); + auto const fullPath = fixture.logFile(); if (withFile == WithFile::no) { @@ -580,9 +502,9 @@ public: // Exercise the jobs interfaces of PerfLog. // Start up the PerfLog that we'll use for testing. - PerfLogParent parent{j_}; - auto perfLog{getPerfLog(parent, withFile)}; - parent.doStart(); + Fixture fixture{j_}; + auto perfLog{fixture.perfLog(withFile)}; + perfLog->start(); // Get the all the JobTypes we can use to call the jobs interfaces // without causing an assert. @@ -879,14 +801,13 @@ public: validateFinalCurrent(perfLog->currentJson()); // Give the PerfLog enough time to flush it's state to the file. - waitForFileUpdate(parent); + fixture.wait(); // Politely stop the PerfLog. - parent.doStop(); + perfLog->stop(); // Check file contents if that is appropriate. - auto const fullPath = - parent.getPerfLogPath() / parent.getPerfLogFileName(); + auto const fullPath = fixture.logFile(); if (withFile == WithFile::no) { @@ -928,9 +849,9 @@ public: // the PerLog behaves as well as possible if an invalid ID is passed. // Start up the PerfLog that we'll use for testing. - PerfLogParent parent{j_}; - auto perfLog{getPerfLog(parent, withFile)}; - parent.doStart(); + Fixture fixture{j_}; + auto perfLog{fixture.perfLog(withFile)}; + perfLog->start(); // Randomly select a job type and its name. JobType jobType; @@ -1021,14 +942,13 @@ public: verifyEmptyCurrent(perfLog->currentJson()); // Give the PerfLog enough time to flush it's state to the file. - waitForFileUpdate(parent); + fixture.wait(); // Politely stop the PerfLog. - parent.doStop(); + perfLog->stop(); // Check file contents if that is appropriate. - auto const fullPath = - parent.getPerfLogPath() / parent.getPerfLogFileName(); + auto const fullPath = fixture.logFile(); if (withFile == WithFile::no) { @@ -1069,54 +989,51 @@ public: // the interface and see that it doesn't crash. using namespace boost::filesystem; - auto const perfLogPath{PerfLogParent::getPerfLogPath()}; - auto const fullPath = perfLogPath / PerfLogParent::getPerfLogFileName(); + Fixture fixture{j_}; + BEAST_EXPECT(!exists(fixture.logDir())); - PerfLogParent parent{j_}; - BEAST_EXPECT(!exists(perfLogPath)); + auto perfLog{fixture.perfLog(withFile)}; - auto perfLog{getPerfLog(parent, withFile)}; - - BEAST_EXPECT(parent.stopSignaled == false); + BEAST_EXPECT(fixture.stopSignaled == false); if (withFile == WithFile::no) { - BEAST_EXPECT(!exists(perfLogPath)); + BEAST_EXPECT(!exists(fixture.logDir())); } else { - BEAST_EXPECT(exists(fullPath)); - BEAST_EXPECT(file_size(fullPath) == 0); + BEAST_EXPECT(exists(fixture.logFile())); + BEAST_EXPECT(file_size(fixture.logFile()) == 0); } // Start PerfLog and wait long enough for PerfLog::report() // to write to its file. - parent.doStart(); - waitForFileUpdate(parent); + perfLog->start(); + fixture.wait(); - decltype(file_size(fullPath)) firstFileSize{0}; + decltype(file_size(fixture.logFile())) firstFileSize{0}; if (withFile == WithFile::no) { - BEAST_EXPECT(!exists(perfLogPath)); + BEAST_EXPECT(!exists(fixture.logDir())); } else { - firstFileSize = file_size(fullPath); + firstFileSize = file_size(fixture.logFile()); BEAST_EXPECT(firstFileSize > 0); } // Rotate and then wait to make sure more stuff is written to the file. perfLog->rotate(); - waitForFileUpdate(parent); + fixture.wait(); - parent.doStop(); + perfLog->stop(); if (withFile == WithFile::no) { - BEAST_EXPECT(!exists(perfLogPath)); + BEAST_EXPECT(!exists(fixture.logDir())); } else { - BEAST_EXPECT(file_size(fullPath) > firstFileSize); + BEAST_EXPECT(file_size(fixture.logFile()) > firstFileSize); } } diff --git a/src/test/core/JobQueue_test.cpp b/src/test/core/JobQueue_test.cpp index 2e85cebe24..d0c698099c 100644 --- a/src/test/core/JobQueue_test.cpp +++ b/src/test/core/JobQueue_test.cpp @@ -47,13 +47,11 @@ class JobQueue_test : public beast::unit_test::suite ; } { - // If the JobQueue's JobCounter is join()ed we should no + // If the JobQueue is stopped, we should no // longer be able to add Jobs (and calling addJob() should // return false). using namespace std::chrono_literals; - beast::Journal j{env.app().journal("JobQueue_test")}; - JobCounter& jCounter = jQueue.jobCounter(); - jCounter.join("JobQueue_test", 1s, j); + jQueue.stop(); // The Job should never run, so having the Job access this // unprotected variable on the stack should be completely safe. @@ -132,13 +130,11 @@ class JobQueue_test : public beast::unit_test::suite BEAST_EXPECT(yieldCount == 4); } { - // If the JobQueue's JobCounter is join()ed we should no + // If the JobQueue is stopped, we should no // longer be able to add a Coro (and calling postCoro() should // return false). using namespace std::chrono_literals; - beast::Journal j{env.app().journal("JobQueue_test")}; - JobCounter& jCounter = jQueue.jobCounter(); - jCounter.join("JobQueue_test", 1s, j); + jQueue.stop(); // The Coro should never run, so having the Coro access this // unprotected variable on the stack should be completely safe. diff --git a/src/test/core/Stoppable_test.cpp b/src/test/core/Stoppable_test.cpp deleted file mode 100644 index c538af2a9c..0000000000 --- a/src/test/core/Stoppable_test.cpp +++ /dev/null @@ -1,539 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. - - Permission to use, copy, modify, and/or 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 -#include -#include -#include - -namespace ripple { -namespace test { - -class Stoppable_test : public beast::unit_test::suite -{ - /* - R - / | \ - / | \ - A B C - / | \ /\ | - D E F G H I - | - J - */ - unsigned count = 0; - - class D : public Stoppable - { - Stoppable_test& test_; - - public: - D(Stoppable& parent, Stoppable_test& test) - : Stoppable("D", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 9, "D::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 0, "D::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 11, "D::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 2, "D::onChildrenStopped called out of order"); - } - }; - - class J : public Stoppable - { - Stoppable_test& test_; - - public: - J(Stoppable& parent, Stoppable_test& test) - : Stoppable("J", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 7, "J::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 1, "J::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 10, "J::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 4, "J::onChildrenStopped called out of order"); - } - }; - - class E : public Stoppable - { - J j_; - Stoppable_test& test_; - - public: - E(Stoppable& parent, Stoppable_test& test) - : Stoppable("E", parent), j_(*this, test), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 8, "E::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 2, "E::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 9, "E::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 3, "E::onChildrenStopped called out of order"); - } - }; - - class F : public Stoppable - { - Stoppable_test& test_; - - public: - F(Stoppable& parent, Stoppable_test& test) - : Stoppable("F", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 6, "F::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 3, "F::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 8, "F::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 5, "F::onChildrenStopped called out of order"); - } - }; - - class A : public Stoppable - { - enum { running, please_stop, stopping, stopped }; - D d_; - E e_; - F f_; - Stoppable_test& test_; - std::atomic stop_; - - public: - A(Stoppable& parent, Stoppable_test& test) - : Stoppable("A", parent) - , d_(*this, test) - , e_(*this, test) - , f_(*this, test) - , test_(test) - , stop_(running) - { - } - ~A() override - { - while (stop_ != stopped) - ; - } - - void - run() - { - while (stop_ == running) - ; - stop_ = stopping; - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 10, "A::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 4, "A::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 7, "A::onStop called out of order"); - } - - void - onChildrenStopped() override - { - stop_ = please_stop; - while (stop_ != stopping) - ; - Stoppable::stopped(); - test_.expect( - --test_.count == 1, "A::onChildrenStopped called out of order"); - stop_ = stopped; - } - }; - - class G : public Stoppable - { - Stoppable_test& test_; - - public: - G(Stoppable& parent, Stoppable_test& test) - : Stoppable("G", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 4, "G::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 5, "G::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 6, "G::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 7, "G::onChildrenStopped called out of order"); - } - }; - - class H : public Stoppable - { - Stoppable_test& test_; - - public: - H(Stoppable& parent, Stoppable_test& test) - : Stoppable("H", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 3, "H::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 6, "H::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 5, "H::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 8, "H::onChildrenStopped called out of order"); - } - }; - - class B : public Stoppable - { - G g_; - H h_; - Stoppable_test& test_; - - public: - B(Stoppable& parent, Stoppable_test& test) - : Stoppable("B", parent) - , g_(*this, test) - , h_(*this, test) - , test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 5, "B::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 7, "B::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 4, "B::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 6, "B::onChildrenStopped called out of order"); - } - }; - - class I : public Stoppable - { - Stoppable_test& test_; - - public: - I(Stoppable& parent, Stoppable_test& test) - : Stoppable("I", parent), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 1, "I::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 8, "I::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 3, "I::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 10, - "I::onChildrenStopped called out of order"); - } - }; - - class C : public Stoppable - { - I i_; - Stoppable_test& test_; - - public: - C(Stoppable& parent, Stoppable_test& test) - : Stoppable("C", parent), i_(*this, test), test_(test) - { - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 2, "C::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect(--test_.count == 9, "C::onStart called out of order"); - } - - void - onStop() override - { - test_.expect(++test_.count == 2, "C::onStop called out of order"); - } - - void - onChildrenStopped() override - { - Stoppable::stopped(); - test_.expect( - --test_.count == 9, "C::onChildrenStopped called out of order"); - } - }; - - class Root : public RootStoppable - { - std::thread a_; - B b_; - C c_; - Stoppable_test& test_; - SuiteJournal journal_; - - public: - explicit Root(Stoppable_test& test) - : RootStoppable("R") - , a_(&A::run, std::make_unique(*this, test)) - , b_(*this, test) - , c_(*this, test) - , test_(test) - , journal_("Stoppable_test", test) - { - } - - void - run() - { - start(); - stop(journal_); - } - - void - onPrepare() override - { - test_.expect( - ++test_.count == 11, "Root::onPrepare called out of order"); - } - - void - onStart() override - { - test_.expect( - --test_.count == 10, "Root::onStart called out of order"); - } - - void - onStop() override - { - test_.expect( - ++test_.count == 1, "Root::onStop called out of order"); - } - - void - onChildrenStopped() override - { - a_.join(); - Stoppable::stopped(); - test_.expect( - --test_.count == 0, - "Root::onChildrenStopped called out of order"); - } - - void - secondStop() - { - // Calling stop() a second time should have no negative - // consequences. - stop(journal_); - } - }; - -public: - void - run() override - { - { - Root rt(*this); - rt.run(); - rt.secondStop(); - } - pass(); - } -}; - -BEAST_DEFINE_TESTSUITE(Stoppable, core, ripple); - -} // namespace test -} // namespace ripple diff --git a/src/test/core/Workers_test.cpp b/src/test/core/Workers_test.cpp index 7243732af1..155dd14f03 100644 --- a/src/test/core/Workers_test.cpp +++ b/src/test/core/Workers_test.cpp @@ -156,7 +156,7 @@ public: testForThreadCount(tc1); testForThreadCount(tc2); testForThreadCount(tc3); - w.pauseAllThreadsAndWait(); + w.stop(); // We had better finished all our work! BEAST_EXPECT(cb.count == 0); diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index d6ca9d82e6..e219928e12 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -86,7 +86,7 @@ Env::AppBundle::AppBundle( if (!app->setup()) Throw("Env::AppBundle: setup failed"); timeKeeper->set(app->getLedgerMaster().getClosedLedger()->info().closeTime); - app->doStart(false /*don't start timers*/); + app->start(false /*don't start timers*/); thread = std::thread([&]() { app->run(); }); client = makeJSONRPCClient(app->config()); diff --git a/src/test/net/DatabaseDownloader_test.cpp b/src/test/net/DatabaseDownloader_test.cpp index c15f8e9b9e..749000dc50 100644 --- a/src/test/net/DatabaseDownloader_test.cpp +++ b/src/test/net/DatabaseDownloader_test.cpp @@ -95,7 +95,7 @@ class DatabaseDownloader_test : public beast::unit_test::suite ~Downloader() { - ptr_->onStop(); + ptr_->stop(); } DatabaseDownloader* diff --git a/src/test/nodestore/DatabaseShard_test.cpp b/src/test/nodestore/DatabaseShard_test.cpp index f8babe9f55..824e1ae264 100644 --- a/src/test/nodestore/DatabaseShard_test.cpp +++ b/src/test/nodestore/DatabaseShard_test.cpp @@ -655,10 +655,9 @@ class DatabaseShard_test : public TestBase beast::temp_dir shardDir; Env env{*this, testConfig(shardDir.path())}; DummyScheduler scheduler; - RootStoppable parent("TestRootStoppable"); std::unique_ptr db = - make_ShardStore(env.app(), parent, scheduler, 2, journal_); + make_ShardStore(env.app(), scheduler, 2, journal_); BEAST_EXPECT(db); BEAST_EXPECT(db->ledgersPerShard() == db->ledgersPerShardDefault); diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index 67d3a6b374..dd8af68c78 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -450,7 +450,6 @@ public: std::int64_t seedValue) { DummyScheduler scheduler; - RootStoppable parent("TestRootStoppable"); beast::temp_dir node_db; Section srcParams; @@ -463,13 +462,7 @@ public: // Write to source db { std::unique_ptr src = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - srcParams, - journal_); + megabytes(4), scheduler, 2, srcParams, journal_); storeBatch(*src, batch); } @@ -478,13 +471,7 @@ public: { // Re-open the db std::unique_ptr src = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - srcParams, - journal_); + megabytes(4), scheduler, 2, srcParams, journal_); // Set up the destination database beast::temp_dir dest_db; @@ -493,13 +480,7 @@ public: destParams.set("path", dest_db.path()); std::unique_ptr dest = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - destParams, - journal_); + megabytes(4), scheduler, 2, destParams, journal_); testcase( "import into '" + destBackendType + "' from '" + @@ -528,7 +509,6 @@ public: int numObjsToTest = 2000) { DummyScheduler scheduler; - RootStoppable parent("TestRootStoppable"); std::string s = "NodeStore backend '" + type + "'"; @@ -547,13 +527,7 @@ public: { // Open the database std::unique_ptr db = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); // Write the batch storeBatch(*db, batch); @@ -578,13 +552,7 @@ public: { // Re-open the database without the ephemeral DB std::unique_ptr db = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); // Read it back in Batch copy; @@ -603,13 +571,7 @@ public: // Verify default earliest ledger sequence std::unique_ptr db = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); BEAST_EXPECT( db->earliestLedgerSeq() == XRP_LEDGER_EARLIEST_SEQ); } @@ -620,13 +582,7 @@ public: nodeParams.set("earliest_seq", "0"); std::unique_ptr db = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); } catch (std::runtime_error const& e) { @@ -639,13 +595,7 @@ public: nodeParams.set("earliest_seq", "1"); std::unique_ptr db = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); // Verify database uses the earliest ledger sequence setting BEAST_EXPECT(db->earliestLedgerSeq() == 1); @@ -659,13 +609,7 @@ public: "earliest_seq", std::to_string(XRP_LEDGER_EARLIEST_SEQ)); std::unique_ptr db2 = Manager::instance().make_Database( - "test", - megabytes(4), - scheduler, - 2, - parent, - nodeParams, - journal_); + megabytes(4), scheduler, 2, nodeParams, journal_); } catch (std::runtime_error const& e) { diff --git a/src/test/rpc/Subscribe_test.cpp b/src/test/rpc/Subscribe_test.cpp index d1d5e6e611..3e1916caf4 100644 --- a/src/test/rpc/Subscribe_test.cpp +++ b/src/test/rpc/Subscribe_test.cpp @@ -61,7 +61,7 @@ public: // first findMsg but BEFORE we unsubscribe, thus causing the final // findMsg check to fail since there is one unprocessed ws msg created // by the loadmanager - env.app().getLoadManager().onStop(); + env.app().getLoadManager().stop(); { // Raise fee to cause an update auto& feeTrack = env.app().getFeeTrack(); diff --git a/src/test/shamap/common.h b/src/test/shamap/common.h index 7a13bb8140..59cee49fbc 100644 --- a/src/test/shamap/common.h +++ b/src/test/shamap/common.h @@ -39,7 +39,6 @@ private: TestStopwatch clock_; NodeStore::DummyScheduler scheduler_; - RootStoppable parent_; beast::Journal const j_; @@ -54,14 +53,13 @@ public: std::chrono::minutes{1}, clock_, j)) - , parent_("TestRootStoppable") , j_(j) { Section testSection; testSection.set("type", "memory"); testSection.set("Path", "SHAMap_test"); db_ = NodeStore::Manager::instance().make_Database( - "test", megabytes(4), scheduler_, 1, parent_, testSection, j); + megabytes(4), scheduler_, 1, testSection, j); } NodeStore::Database&