From 8e6a0d418ce306b62b757dfa7b646d183c1830a4 Mon Sep 17 00:00:00 2001 From: seelabs Date: Wed, 1 Jun 2022 11:58:27 -0400 Subject: [PATCH] Fix race conditions in shard: ThreadSafetyAnalysis was used to identify race conditions in this file. This analysis was modivated by a (rare) crash while running unit tests. Add locks to Shard flagged by ThreadSafetyAnalysis --- src/ripple/nodestore/impl/Shard.cpp | 20 ++++++++++++++++---- src/ripple/nodestore/impl/Shard.h | 26 +++++++++++++++----------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 911eedef6b..030fbf4aa1 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -223,6 +223,7 @@ Shard::storeNodeObject(std::shared_ptr const& nodeObject) try { + std::lock_guard lock(mutex_); backend_->store(nodeObject); } catch (std::exception const& e) @@ -249,6 +250,7 @@ Shard::fetchNodeObject(uint256 const& hash, FetchReport& fetchReport) Status status; try { + std::lock_guard lock(mutex_); status = backend_->fetch(hash.data(), &nodeObject); } catch (std::exception const& e) @@ -331,6 +333,7 @@ Shard::storeLedger( try { + std::lock_guard lock(mutex_); backend_->storeBatch(batch); } catch (std::exception const& e) @@ -538,6 +541,7 @@ Shard::getWriteLoad() auto const scopedCount{makeBackendCount()}; if (!scopedCount) return 0; + std::lock_guard lock(mutex_); return backend_->getWriteLoad(); } @@ -572,6 +576,8 @@ Shard::finalize(bool writeSQLite, std::optional const& referenceHash) try { + std::lock_guard lock(mutex_); + state_ = ShardState::finalizing; progress_ = 0; @@ -759,8 +765,11 @@ Shard::finalize(bool writeSQLite, std::optional const& referenceHash) try { - // Store final key's value, may already be stored - backend_->store(nodeObject); + { + // Store final key's value, may already be stored + std::lock_guard lock(mutex_); + backend_->store(nodeObject); + } // Do not allow all other threads work with the shard busy_ = true; @@ -819,7 +828,7 @@ Shard::open(std::lock_guard const& lock) using namespace boost::filesystem; Config const& config{app_.config()}; auto preexist{false}; - auto fail = [this, &preexist](std::string const& msg) { + auto fail = [this, &preexist](std::string const& msg) REQUIRES(mutex_) { backend_->close(); lgrSQLiteDB_.reset(); txSQLiteDB_.reset(); @@ -837,7 +846,7 @@ Shard::open(std::lock_guard const& lock) } return false; }; - auto createAcquireInfo = [this, &config]() { + auto createAcquireInfo = [this, &config]() REQUIRES(mutex_) { DatabaseCon::Setup setup; setup.startUp = config.standalone() ? config.LOAD : config.START_UP; setup.standAlone = config.standalone(); @@ -1024,6 +1033,8 @@ Shard::storeSQLite(std::shared_ptr const& ledger) try { + std::lock_guard lock(mutex_); + auto res = updateLedgerDBs( *txSQLiteDB_->checkoutDb(), *lgrSQLiteDB_->checkoutDb(), @@ -1186,6 +1197,7 @@ Shard::verifyFetch(uint256 const& hash) const try { + std::lock_guard lock(mutex_); switch (backend_->fetch(hash.data(), &nodeObject)) { case ok: diff --git a/src/ripple/nodestore/impl/Shard.h b/src/ripple/nodestore/impl/Shard.h index b7516e5f1e..210bdd54a6 100644 --- a/src/ripple/nodestore/impl/Shard.h +++ b/src/ripple/nodestore/impl/Shard.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -210,6 +211,7 @@ public: std::string getStoredSeqs() { + std::lock_guard lock(mutex_); if (!acquireInfo_) return ""; @@ -316,29 +318,30 @@ private: boost::filesystem::path const dir_; // Storage space utilized by the shard - std::uint64_t fileSz_{0}; + GUARDED_BY(mutex_) std::uint64_t fileSz_{0}; // Number of file descriptors required by the shard - std::uint32_t fdRequired_{0}; + GUARDED_BY(mutex_) std::uint32_t fdRequired_{0}; // NuDB key/value store for node objects - std::unique_ptr backend_; + std::unique_ptr backend_ GUARDED_BY(mutex_); std::atomic backendCount_{0}; // Ledger SQLite database used for indexes - std::unique_ptr lgrSQLiteDB_; + std::unique_ptr lgrSQLiteDB_ GUARDED_BY(mutex_); // Transaction SQLite database used for indexes - std::unique_ptr txSQLiteDB_; + std::unique_ptr txSQLiteDB_ GUARDED_BY(mutex_); // Tracking information used only when acquiring a shard from the network. // If the shard is finalized, this member will be null. - std::unique_ptr acquireInfo_; + std::unique_ptr acquireInfo_ GUARDED_BY(mutex_); + ; // Older shard without an acquire database or final key // Eventually there will be no need for this and should be removed - bool legacy_{false}; + GUARDED_BY(mutex_) bool legacy_{false}; // Determines if the shard needs to stop processing for shutdown std::atomic stop_{false}; @@ -356,16 +359,17 @@ private: std::atomic removeOnDestroy_{false}; // The time of the last access of a shard with a finalized state - std::chrono::steady_clock::time_point lastAccess_; + std::chrono::steady_clock::time_point lastAccess_ GUARDED_BY(mutex_); + ; // Open shard databases [[nodiscard]] bool - open(std::lock_guard const& lock); + open(std::lock_guard const& lock) REQUIRES(mutex_); // Open/Create SQLite databases // Lock over mutex_ required [[nodiscard]] bool - initSQLite(std::lock_guard const&); + initSQLite(std::lock_guard const&) REQUIRES(mutex_); // Write SQLite entries for this ledger [[nodiscard]] bool @@ -374,7 +378,7 @@ private: // Set storage and file descriptor usage stats // Lock over mutex_ required void - setFileStats(std::lock_guard const&); + setFileStats(std::lock_guard const&) REQUIRES(mutex_); // Verify this ledger by walking its SHAMaps and verifying its Merkle trees // Every node object verified will be stored in the deterministic shard