From 8268162cacc7b9fdd76fd7c5be69122a2001ae99 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Fri, 11 Mar 2016 14:04:21 -0500 Subject: [PATCH] Fix SHAMapStore test timing consistency --- src/ripple/app/misc/SHAMapStore.h | 2 + src/ripple/app/misc/SHAMapStoreImp.cpp | 20 +++- src/ripple/app/misc/SHAMapStoreImp.h | 13 +- src/ripple/app/tests/SHAMapStore_test.cpp | 139 ++++++++++------------ 4 files changed, 87 insertions(+), 87 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStore.h b/src/ripple/app/misc/SHAMapStore.h index ad85c87cd..8ffe34c7e 100644 --- a/src/ripple/app/misc/SHAMapStore.h +++ b/src/ripple/app/misc/SHAMapStore.h @@ -57,6 +57,8 @@ public: /** Called by LedgerMaster every time a ledger validates. */ virtual void onLedgerClosed(std::shared_ptr const& ledger) = 0; + virtual void rendezvous() const = 0; + virtual std::uint32_t clampFetchDepth (std::uint32_t fetch_depth) const = 0; virtual std::unique_ptr makeDatabase ( diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 4ab49000a..c925e9384 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -180,7 +180,7 @@ SHAMapStoreImp::SHAMapStoreImp ( , scheduler_ (scheduler) , journal_ (journal) , nodeStoreJournal_ (nodeStoreJournal) - , rotating_(false) + , working_(true) , transactionMaster_ (transactionMaster) , canDelete_ (std::numeric_limits ::max()) { @@ -251,10 +251,24 @@ SHAMapStoreImp::onLedgerClosed( { std::lock_guard lock (mutex_); newLedger_ = ledger; + working_ = true; } cond_.notify_one(); } +void +SHAMapStoreImp::rendezvous() const +{ + if (!working_) + return; + + std::unique_lock lock(mutex_); + rendezvous_.wait(lock, [&] + { + return !working_; + }); +} + bool SHAMapStoreImp::copyNode (std::uint64_t& nodeCount, SHAMapAbstractNode const& node) @@ -288,10 +302,11 @@ SHAMapStoreImp::run() { healthy_ = true; std::shared_ptr validatedLedger; - rotating_ = false; { std::unique_lock lock (mutex_); + working_ = false; + rendezvous_.notify_all(); if (stop_) { stopped(); @@ -300,7 +315,6 @@ SHAMapStoreImp::run() cond_.wait (lock); if (newLedger_) { - rotating_ = true; validatedLedger = std::move(newLedger_); } else diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index 797db777c..344b8486b 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -20,9 +20,9 @@ #ifndef RIPPLE_APP_MISC_SHAMAPSTOREIMP_H_INCLUDED #define RIPPLE_APP_MISC_SHAMAPSTOREIMP_H_INCLUDED -#include #include #include +#include #include #include #include @@ -94,9 +94,10 @@ private: bool stop_ = false; bool healthy_ = true; mutable std::condition_variable cond_; + mutable std::condition_variable rendezvous_; mutable std::mutex mutex_; std::shared_ptr newLedger_; - std::atomic rotating_; + std::atomic working_; TransactionMaster& transactionMaster_; std::atomic canDelete_; // these do not exist upon SHAMapStore creation, but do exist @@ -108,12 +109,6 @@ private: DatabaseCon* transactionDb_ = nullptr; DatabaseCon* ledgerDb_ = nullptr; -public: - bool rotating() const - { - return rotating_; - } - public: SHAMapStoreImp (Application& app, Setup const& setup, @@ -168,6 +163,8 @@ public: void onLedgerClosed (std::shared_ptr const& ledger) override; + void rendezvous() const override; + private: // callback for visitNodes bool copyNode (std::uint64_t& nodeCount, SHAMapAbstractNode const &node); diff --git a/src/ripple/app/tests/SHAMapStore_test.cpp b/src/ripple/app/tests/SHAMapStore_test.cpp index 7d9ccd241..bd0494ab8 100644 --- a/src/ripple/app/tests/SHAMapStore_test.cpp +++ b/src/ripple/app/tests/SHAMapStore_test.cpp @@ -19,8 +19,10 @@ #include #include -#include +#include #include +#include +#include #include #include @@ -188,14 +190,16 @@ class SHAMapStore_test : public beast::unit_test::suite auto& store = env.app().getSHAMapStore(); int ledgerSeq = 3; - while (!store.getLastRotated()) - { - env.close(); - std::this_thread::sleep_for(100ms); + store.rendezvous(); + expect(!store.getLastRotated()); - auto ledger = env.rpc("ledger", "validated"); - expect(goodLedger(env, ledger, to_string(ledgerSeq++))); - } + env.close(); + store.rendezvous(); + + auto ledger = env.rpc("ledger", "validated"); + expect(goodLedger(env, ledger, to_string(ledgerSeq++))); + + expect(store.getLastRotated() == ledgerSeq - 1); return ledgerSeq; } @@ -209,9 +213,7 @@ public: Env env(*this, makeConfig()); - auto store = dynamic_cast( - &env.app().getSHAMapStore()); - expect(store); + auto& store = env.app().getSHAMapStore(); env.fund(XRP(10000), noripple("alice")); validationCheck(env, 0); @@ -239,7 +241,10 @@ public: ledgerTmp = env.rpc("ledger", "100"); expect(bad(ledgerTmp)); - for (auto i = 4; i < deleteInterval + 4; ++i) + auto const firstSeq = waitForReady(env); + auto lastRotated = firstSeq - 1; + + for (auto i = firstSeq + 1; i < deleteInterval + firstSeq; ++i) { env.fund(XRP(10000), noripple("test" + to_string(i))); env.close(); @@ -247,9 +252,9 @@ public: ledgerTmp = env.rpc("ledger", "current"); expect(goodLedger(env, ledgerTmp, to_string(i))); } - assert(store->getLastRotated() == 3); + expect(store.getLastRotated() == lastRotated); - for (auto i = 3; i < deleteInterval + 3; ++i) + for (auto i = 3; i < deleteInterval + lastRotated; ++i) { ledgers.emplace(std::make_pair(i, env.rpc("ledger", to_string(i)))); @@ -259,8 +264,8 @@ public: validationCheck(env, 0); ledgerCheck(env, deleteInterval + 1, 2); - transactionCheck(env, deleteInterval + 1); - accountTransactionCheck(env, 2*(deleteInterval + 1)); + transactionCheck(env, deleteInterval); + accountTransactionCheck(env, 2 * deleteInterval); { // Since standalone doesn't _do_ validations, manually @@ -319,8 +324,8 @@ public: validationCheck(env, deleteInterval + 23); ledgerCheck(env, deleteInterval + 1, 2); - transactionCheck(env, deleteInterval + 1); - accountTransactionCheck(env, 2 * (deleteInterval + 1)); + transactionCheck(env, deleteInterval); + accountTransactionCheck(env, 2 * deleteInterval); { // Closing one more ledger triggers a rotate @@ -330,18 +335,17 @@ public: expect(goodLedger(env, ledger, to_string(deleteInterval + 4))); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); - expect(store->getLastRotated() == deleteInterval + 3); - auto const lastRotated = store->getLastRotated(); + expect(store.getLastRotated() == deleteInterval + 3); + lastRotated = store.getLastRotated(); expect(lastRotated == 11, to_string(lastRotated)); // That took care of the fake hashes validationCheck(env, deleteInterval + 8); ledgerCheck(env, deleteInterval + 1, 3); - transactionCheck(env, deleteInterval + 1); - accountTransactionCheck(env, 2 * (deleteInterval + 1)); + transactionCheck(env, deleteInterval); + accountTransactionCheck(env, 2 * deleteInterval); // The last iteration of this loop should trigger a rotate for (auto i = lastRotated - 1; i < lastRotated + deleteInterval - 1; ++i) @@ -355,7 +359,7 @@ public: ledgers.emplace(std::make_pair(i, env.rpc("ledger", to_string(i)))); - expect(store->getLastRotated() == lastRotated || + expect(store.getLastRotated() == lastRotated || i == lastRotated + deleteInterval - 2, "lastRotated"); expect(goodLedger(env, ledgers[i], to_string(i), true) && getHash(ledgers[i]).length(), to_string(ledgers[i])); @@ -376,10 +380,9 @@ public: soci::use(ledgerSeqs); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); - expect(store->getLastRotated() == deleteInterval + lastRotated); + expect(store.getLastRotated() == deleteInterval + lastRotated); validationCheck(env, deleteInterval - 1); ledgerCheck(env, deleteInterval + 1, lastRotated); @@ -395,14 +398,12 @@ public: using namespace std::chrono_literals; Env env(*this, makeConfig()); - auto store = dynamic_cast( - &env.app().getSHAMapStore()); - expect(store); + auto& store = env.app().getSHAMapStore(); auto ledgerSeq = waitForReady(env); auto lastRotated = ledgerSeq - 1; - expect(store->getLastRotated() == lastRotated, - to_string(store->getLastRotated())); + expect(store.getLastRotated() == lastRotated, + to_string(store.getLastRotated())); expect(lastRotated != 2); // Because advisory_delete is unset, @@ -419,14 +420,13 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while(store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); // The database will always have back to ledger 2, // regardless of lastRotated. validationCheck(env, 0); ledgerCheck(env, ledgerSeq - 2, 2); - expect(lastRotated == store->getLastRotated()); + expect(lastRotated == store.getLastRotated()); { // Closing one more ledger triggers a rotate @@ -436,14 +436,13 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq++), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); - expect(lastRotated != store->getLastRotated()); + expect(lastRotated != store.getLastRotated()); - lastRotated = store->getLastRotated(); + lastRotated = store.getLastRotated(); // Close enough ledgers to trigger another rotate for (; ledgerSeq < lastRotated + deleteInterval + 1; ++ledgerSeq) @@ -454,12 +453,11 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, deleteInterval + 1, lastRotated); - expect(lastRotated != store->getLastRotated()); + expect(lastRotated != store.getLastRotated()); } void testCanDelete() @@ -470,14 +468,12 @@ public: // Same config with advisory_delete enabled Env env(*this, makeConfigAdvisory()); - auto store = dynamic_cast( - &env.app().getSHAMapStore()); - expect(store); + auto& store = env.app().getSHAMapStore(); auto ledgerSeq = waitForReady(env); auto lastRotated = ledgerSeq - 1; - expect(store->getLastRotated() == lastRotated, - to_string(store->getLastRotated())); + expect(store.getLastRotated() == lastRotated, + to_string(store.getLastRotated())); expect(lastRotated != 2); auto canDelete = env.rpc("can_delete"); @@ -497,12 +493,11 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - 2, 2); - expect(lastRotated == store->getLastRotated()); + expect(lastRotated == store.getLastRotated()); // This does not kick off a cleanup canDelete = env.rpc("can_delete", to_string( @@ -511,12 +506,11 @@ public: expect(canDelete[jss::result][jss::can_delete] == ledgerSeq + deleteInterval / 2); - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - 2, 2); - expect(store->getLastRotated() == lastRotated); + expect(store.getLastRotated() == lastRotated); { // This kicks off a cleanup, but it stays small. @@ -526,13 +520,12 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq++), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); - expect(store->getLastRotated() == ledgerSeq - 1); + expect(store.getLastRotated() == ledgerSeq - 1); lastRotated = ledgerSeq - 1; for (; ledgerSeq < lastRotated + deleteInterval; ++ledgerSeq) @@ -544,10 +537,9 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); - expect(store->getLastRotated() == lastRotated); + expect(store.getLastRotated() == lastRotated); { // This kicks off another cleanup. @@ -557,13 +549,12 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq++), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - firstBatch, firstBatch); - expect(store->getLastRotated() == ledgerSeq - 1); + expect(store.getLastRotated() == ledgerSeq - 1); lastRotated = ledgerSeq - 1; // This does not kick off a cleanup @@ -581,10 +572,9 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); - expect(store->getLastRotated() == lastRotated); + expect(store.getLastRotated() == lastRotated); { // This kicks off another cleanup. @@ -594,13 +584,12 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq++), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); - expect(store->getLastRotated() == ledgerSeq - 1); + expect(store.getLastRotated() == ledgerSeq - 1); lastRotated = ledgerSeq - 1; // This does not kick off a cleanup @@ -617,10 +606,9 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); - expect(store->getLastRotated() == lastRotated); + expect(store.getLastRotated() == lastRotated); { // This kicks off another cleanup. @@ -630,13 +618,12 @@ public: expect(goodLedger(env, ledger, to_string(ledgerSeq++), true)); } - while (store->rotating()) - std::this_thread::sleep_for(1ms); + store.rendezvous(); validationCheck(env, 0); ledgerCheck(env, ledgerSeq - lastRotated, lastRotated); - expect(store->getLastRotated() == ledgerSeq - 1); + expect(store.getLastRotated() == ledgerSeq - 1); lastRotated = ledgerSeq - 1; } @@ -649,7 +636,7 @@ public: }; // VFALCO This test fails because of thread asynchronous issues -BEAST_DEFINE_TESTSUITE_MANUAL(SHAMapStore,app,ripple); +BEAST_DEFINE_TESTSUITE(SHAMapStore,app,ripple); } }