diff --git a/Builds/levelization/results/ordering.txt b/Builds/levelization/results/ordering.txt index a9835cc4b..bef03e6cb 100644 --- a/Builds/levelization/results/ordering.txt +++ b/Builds/levelization/results/ordering.txt @@ -18,6 +18,7 @@ test.app > xrpl.basics test.app > xrpld.app test.app > xrpld.core test.app > xrpld.ledger +test.app > xrpld.nodestore test.app > xrpld.overlay test.app > xrpld.rpc test.app > xrpl.hook diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index 376cb4eb7..b212c7568 100644 --- a/src/test/app/SHAMapStore_test.cpp +++ b/src/test/app/SHAMapStore_test.cpp @@ -20,9 +20,11 @@ #include #include #include +#include #include #include #include +#include #include namespace ripple { @@ -518,12 +520,137 @@ public: lastRotated = ledgerSeq - 1; } + std::unique_ptr + makeBackendRotating( + jtx::Env& env, + NodeStoreScheduler& scheduler, + std::string path) + { + Section section{ + env.app().config().section(ConfigSection::nodeDatabase())}; + boost::filesystem::path newPath; + + if (!BEAST_EXPECT(path.size())) + return {}; + newPath = path; + section.set("path", newPath.string()); + + auto backend{NodeStore::Manager::instance().make_Backend( + section, + megabytes(env.app().config().getValueFor( + SizedItem::burstSize, std::nullopt)), + scheduler, + env.app().logs().journal("NodeStoreTest"))}; + backend->open(); + return backend; + } + + void + testRotate() + { + // The only purpose of this test is to ensure that if something that + // should never happen happens, we don't get a deadlock. + testcase("rotate with lock contention"); + + using namespace jtx; + Env env(*this, envconfig(onlineDelete)); + + ///////////////////////////////////////////////////////////// + // Create the backend. Normally, SHAMapStoreImp handles all these + // details + auto nscfg = env.app().config().section(ConfigSection::nodeDatabase()); + + // Provide default values: + if (!nscfg.exists("cache_size")) + nscfg.set( + "cache_size", + std::to_string(env.app().config().getValueFor( + SizedItem::treeCacheSize, std::nullopt))); + + if (!nscfg.exists("cache_age")) + nscfg.set( + "cache_age", + std::to_string(env.app().config().getValueFor( + SizedItem::treeCacheAge, std::nullopt))); + + NodeStoreScheduler scheduler(env.app().getJobQueue()); + + std::string const writableDb = "write"; + std::string const archiveDb = "archive"; + auto writableBackend = makeBackendRotating(env, scheduler, writableDb); + auto archiveBackend = makeBackendRotating(env, scheduler, archiveDb); + + // Create NodeStore with two backends to allow online deletion of + // data + constexpr int readThreads = 4; + auto dbr = std::make_unique( + env.app(), + scheduler, + readThreads, + std::move(writableBackend), + std::move(archiveBackend), + nscfg, + env.app().logs().journal("NodeStoreTest")); + + ///////////////////////////////////////////////////////////// + // Check basic functionality + using namespace std::chrono_literals; + std::atomic threadNum = 0; + + { + auto newBackend = makeBackendRotating( + env, scheduler, std::to_string(++threadNum)); + + auto const cb = [&](std::string const& writableName, + std::string const& archiveName) { + BEAST_EXPECT(writableName == "1"); + BEAST_EXPECT(archiveName == "write"); + // Ensure that dbr functions can be called from within the + // callback + BEAST_EXPECT(dbr->getName() == "1"); + }; + + dbr->rotate(std::move(newBackend), cb); + } + BEAST_EXPECT(threadNum == 1); + BEAST_EXPECT(dbr->getName() == "1"); + + ///////////////////////////////////////////////////////////// + // Do something stupid. Try to re-enter rotate from inside the callback. + { + auto const cb = [&](std::string const& writableName, + std::string const& archiveName) { + BEAST_EXPECT(writableName == "3"); + BEAST_EXPECT(archiveName == "2"); + // Ensure that dbr functions can be called from within the + // callback + BEAST_EXPECT(dbr->getName() == "3"); + }; + auto const cbReentrant = [&](std::string const& writableName, + std::string const& archiveName) { + BEAST_EXPECT(writableName == "2"); + BEAST_EXPECT(archiveName == "1"); + auto newBackend = makeBackendRotating( + env, scheduler, std::to_string(++threadNum)); + // Reminder: doing this is stupid and should never happen + dbr->rotate(std::move(newBackend), cb); + }; + auto newBackend = makeBackendRotating( + env, scheduler, std::to_string(++threadNum)); + dbr->rotate(std::move(newBackend), cbReentrant); + } + + BEAST_EXPECT(threadNum == 3); + BEAST_EXPECT(dbr->getName() == "3"); + } + void run() override { testClear(); testAutomatic(); testCanDelete(); + testRotate(); } }; diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index a89cad3c2..aff3569d3 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -369,17 +369,17 @@ SHAMapStoreImp::run() lastRotated = validatedSeq; - dbRotating_->rotateWithLock( - [&](std::string const& writableBackendName) { + dbRotating_->rotate( + std::move(newBackend), + [&](std::string const& writableName, + std::string const& archiveName) { SavedState savedState; - savedState.writableDb = newBackend->getName(); - savedState.archiveDb = writableBackendName; + savedState.writableDb = writableName; + savedState.archiveDb = archiveName; savedState.lastRotated = lastRotated; state_db_.setState(savedState); clearCaches(validatedSeq); - - return std::move(newBackend); }); JLOG(journal_.warn()) << "finished rotation " << validatedSeq; diff --git a/src/xrpld/nodestore/DatabaseRotating.h b/src/xrpld/nodestore/DatabaseRotating.h index 10f575c46..3e8c6a7d5 100644 --- a/src/xrpld/nodestore/DatabaseRotating.h +++ b/src/xrpld/nodestore/DatabaseRotating.h @@ -44,11 +44,17 @@ public: /** Rotates the backends. - @param f A function executed before the rotation and under the same lock + @param newBackend New writable backend + @param f A function executed after the rotation outside of lock. The + values passed to f will be the new backend database names _after_ + rotation. */ virtual void - rotateWithLock(std::function( - std::string const& writableBackendName)> const& f) = 0; + rotate( + std::unique_ptr&& newBackend, + std::function const& f) = 0; }; } // namespace NodeStore diff --git a/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp b/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp index 30b09b5b0..1499976d0 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp @@ -45,67 +45,84 @@ DatabaseRotatingImp::DatabaseRotatingImp( } void -DatabaseRotatingImp::rotateWithLock( - std::function( - std::string const& writableBackendName)> const& f) +DatabaseRotatingImp::rotate( + std::unique_ptr&& newBackend, + std::function const& f) { - std::lock_guard lock(mutex_); + // Pass these two names to the callback function + std::string const newWritableBackendName = newBackend->getName(); + std::string newArchiveBackendName; + // Hold on to current archive backend pointer until after the + // callback finishes. Only then will the archive directory be + // deleted. + std::shared_ptr oldArchiveBackend; + { + std::lock_guard lock(mutex_); - // Create the new backend - auto newBackend = f(writableBackend_->getName()); + // Before rotating, ensure all pinned ledgers are in the writable + // backend + JLOG(j_.info()) + << "Ensuring pinned ledgers are preserved before backend rotation"; - // Before rotating, ensure all pinned ledgers are in the writable backend - JLOG(j_.info()) - << "Ensuring pinned ledgers are preserved before backend rotation"; + // Use a lambda to handle the preservation of pinned ledgers + auto ensurePinnedLedgersInWritable = [this]() { + // Get list of pinned ledgers + auto pinnedLedgers = + app_.getLedgerMaster().getPinnedLedgersRangeSet(); - // Use a lambda to handle the preservation of pinned ledgers - auto ensurePinnedLedgersInWritable = [this]() { - // Get list of pinned ledgers - auto pinnedLedgers = app_.getLedgerMaster().getPinnedLedgersRangeSet(); - - for (auto const& range : pinnedLedgers) - { - for (auto seq = range.lower(); seq <= range.upper(); ++seq) + for (auto const& range : pinnedLedgers) { - uint256 hash = app_.getLedgerMaster().getHashBySeq(seq); - if (hash.isZero()) - continue; + for (auto seq = range.lower(); seq <= range.upper(); ++seq) + { + uint256 hash = app_.getLedgerMaster().getHashBySeq(seq); + if (hash.isZero()) + continue; - // Try to load the ledger - auto ledger = app_.getLedgerMaster().getLedgerByHash(hash); - if (ledger && ledger->isImmutable()) - { - // If we have the ledger, store it in the writable backend - JLOG(j_.debug()) << "Ensuring pinned ledger " << seq - << " is in writable backend"; - // TQ: TODO: check this - Database::storeLedger(*ledger, writableBackend_); - } - else - { - // If we don't have the ledger in memory, try to fetch its - // objects directly - JLOG(j_.debug()) << "Attempting to copy pinned ledger " - << seq << " header to writable backend"; - std::shared_ptr headerObj; - Status status = - archiveBackend_->fetch(hash.data(), &headerObj); - if (status == ok && headerObj) - writableBackend_->store(headerObj); + // Try to load the ledger + auto ledger = app_.getLedgerMaster().getLedgerByHash(hash); + if (ledger && ledger->isImmutable()) + { + // If we have the ledger, store it in the writable + // backend + JLOG(j_.debug()) << "Ensuring pinned ledger " << seq + << " is in writable backend"; + // TQ: TODO: check this + Database::storeLedger(*ledger, writableBackend_); + } + else + { + // If we don't have the ledger in memory, try to fetch + // its objects directly + JLOG(j_.debug()) + << "Attempting to copy pinned ledger " << seq + << " header to writable backend"; + std::shared_ptr headerObj; + Status status = + archiveBackend_->fetch(hash.data(), &headerObj); + if (status == ok && headerObj) + writableBackend_->store(headerObj); + } } } - } - }; + }; - // Execute the lambda - ensurePinnedLedgersInWritable(); + // Execute the lambda + ensurePinnedLedgersInWritable(); - // Now it's safe to mark the archive backend for deletion - archiveBackend_->setDeletePath(); + // Now it's safe to mark the archive backend for deletion + archiveBackend_->setDeletePath(); + oldArchiveBackend = std::move(archiveBackend_); - // Complete the rotation - archiveBackend_ = std::move(writableBackend_); - writableBackend_ = std::move(newBackend); + // Complete the rotation + archiveBackend_ = std::move(writableBackend_); + newArchiveBackendName = archiveBackend_->getName(); + + writableBackend_ = std::move(newBackend); + } + + f(newWritableBackendName, newArchiveBackendName); } std::string diff --git a/src/xrpld/nodestore/detail/DatabaseRotatingImp.h b/src/xrpld/nodestore/detail/DatabaseRotatingImp.h index 69cda798e..9052de07e 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.h +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.h @@ -52,9 +52,11 @@ public: } void - rotateWithLock( - std::function( - std::string const& writableBackendName)> const& f) override; + rotate( + std::unique_ptr&& newBackend, + std::function const& f) override; std::string getName() const override; @@ -88,13 +90,7 @@ public: private: std::shared_ptr writableBackend_; std::shared_ptr archiveBackend_; - // This needs to be a recursive mutex because callbacks in `rotateWithLock` - // can call function that also lock the mutex. A current example of this is - // a callback from SHAMapStoreImp, which calls `clearCaches`. This - // `clearCaches` call eventually calls `fetchNodeObject` which tries to - // relock the mutex. It would be desirable to rewrite the code so the lock - // was not held during a callback. - mutable std::recursive_mutex mutex_; + mutable std::mutex mutex_; std::shared_ptr fetchNodeObject(