diff --git a/Builds/levelization/results/ordering.txt b/Builds/levelization/results/ordering.txt index acf8daafb7..681f76dd5d 100644 --- a/Builds/levelization/results/ordering.txt +++ b/Builds/levelization/results/ordering.txt @@ -19,6 +19,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.json diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index 376cb4eb7b..5fd3f79c9f 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,136 @@ 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( + 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 3a530e0e41..e2e0e3b9c4 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -366,17 +366,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 10f575c466..3e8c6a7d5f 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 58cc3599dc..c7e6c8c349 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp @@ -41,16 +41,32 @@ 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_); - auto newBackend = f(writableBackend_->getName()); - archiveBackend_->setDeletePath(); - archiveBackend_ = std::move(writableBackend_); - writableBackend_ = std::move(newBackend); + archiveBackend_->setDeletePath(); + oldArchiveBackend = std::move(archiveBackend_); + + 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 5183aa1e2e..d9f114f503 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.h +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.h @@ -49,9 +49,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; @@ -82,13 +84,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(