From 0e53105ab5be003a44bd0e45f60d11aac97b39a9 Mon Sep 17 00:00:00 2001 From: David Schwartz Date: Tue, 3 Dec 2013 16:01:08 -0800 Subject: [PATCH] Remove dead code and fix a race condition in the node back end. Race was between a store and a fetch, as follows: 1) Fetch: Search for the node, it is not found. 2) Store: Add the node to the database, remove the node from the negative cache. 3) Fetch: Add the node to the negative cache. This would leave a node in the DB, cache, and negative cache. --- .../nodestore/impl/BatchWriter.cpp | 14 +-- src/ripple_core/nodestore/impl/BatchWriter.h | 1 - src/ripple_core/nodestore/impl/DatabaseImp.h | 116 ++++++++---------- 3 files changed, 52 insertions(+), 79 deletions(-) diff --git a/src/ripple_core/nodestore/impl/BatchWriter.cpp b/src/ripple_core/nodestore/impl/BatchWriter.cpp index d0869fbcf..b4cde862f 100644 --- a/src/ripple_core/nodestore/impl/BatchWriter.cpp +++ b/src/ripple_core/nodestore/impl/BatchWriter.cpp @@ -23,7 +23,6 @@ namespace NodeStore BatchWriter::BatchWriter (Callback& callback, Scheduler& scheduler) : m_callback (callback) , m_scheduler (scheduler) - , mWriteGeneration (0) , mWriteLoad (0) , mWritePending (false) { @@ -63,8 +62,6 @@ void BatchWriter::performScheduledTask () void BatchWriter::writeBatch () { - int setSize = 0; - for (;;) { std::vector< boost::shared_ptr > set; @@ -76,21 +73,17 @@ void BatchWriter::writeBatch () mWriteSet.swap (set); assert (mWriteSet.empty ()); - ++mWriteGeneration; - mWriteCondition.notify_all (); + mWriteLoad = set.size (); if (set.empty ()) { mWritePending = false; - mWriteLoad = 0; + mWriteCondition.notify_all (); // VFALCO NOTE Fix this function to not return from the middle return; } - int newSetSize = mWriteSet.size(); - mWriteLoad = std::max (setSize, newSetSize); - newSetSize = set.size (); } m_callback.writeBatch (set); @@ -100,9 +93,8 @@ void BatchWriter::writeBatch () void BatchWriter::waitForWriting () { LockType::scoped_lock sl (mWriteMutex); - int gen = mWriteGeneration; - while (mWritePending && (mWriteGeneration == gen)) + while (mWritePending) mWriteCondition.wait (sl); } diff --git a/src/ripple_core/nodestore/impl/BatchWriter.h b/src/ripple_core/nodestore/impl/BatchWriter.h index 3c280e94a..033c86f4c 100644 --- a/src/ripple_core/nodestore/impl/BatchWriter.h +++ b/src/ripple_core/nodestore/impl/BatchWriter.h @@ -76,7 +76,6 @@ private: Scheduler& m_scheduler; LockType mWriteMutex; CondvarType mWriteCondition; - int mWriteGeneration; int mWriteLoad; bool mWritePending; Batch mWriteSet; diff --git a/src/ripple_core/nodestore/impl/DatabaseImp.h b/src/ripple_core/nodestore/impl/DatabaseImp.h index 0239cf817..fdcb18ca5 100644 --- a/src/ripple_core/nodestore/impl/DatabaseImp.h +++ b/src/ripple_core/nodestore/impl/DatabaseImp.h @@ -34,7 +34,6 @@ public: , m_fastBackend ((fastBackendParameters.size () > 0) ? createBackend (fastBackendParameters, scheduler) : nullptr) , m_cache ("NodeStore", 16384, 300) - , m_negativeCache ("NoteStoreNegativeCache", 0, 120) { } @@ -57,75 +56,61 @@ public: if (obj == nullptr) { - // It's not in the cache, see if we can skip checking the db. + // There's still a chance it could be in one of the databases. + + bool foundInFastBackend = false; + + // Check the fast backend database if we have one // - if (! m_negativeCache.isPresent (hash)) + if (m_fastBackend != nullptr) { - // There's still a chance it could be in one of the databases. + obj = fetchInternal (m_fastBackend, hash); - bool foundInFastBackend = false; - - // Check the fast backend database if we have one - // - if (m_fastBackend != nullptr) - { - obj = fetchInternal (m_fastBackend, hash); - - // If we found the object, avoid storing it again later. - if (obj != nullptr) - foundInFastBackend = true; - } - - // Are we still without an object? - // - if (obj == nullptr) - { - // Yes so at last we will try the main database. - // - { - // Monitor this operation's load since it is expensive. - // - // VFALCO TODO Why is this an autoptr? Why can't it just be a plain old object? - // - // VFALCO NOTE Commented this out because it breaks the unit test! - // - //LoadEvent::autoptr event (getApp().getJobQueue ().getLoadEventAP (jtHO_READ, "HOS::retrieve")); - - obj = fetchInternal (m_backend, hash); - } - - // If it's not in the main database, remember that so we - // can skip the lookup for the same object again later. - // - if (obj == nullptr) - m_negativeCache.add (hash); - } - - // Did we finally get something? - // + // If we found the object, avoid storing it again later. if (obj != nullptr) - { - // Yes it so canonicalize. This solves the problem where - // more than one thread has its own copy of the same object. - // - m_cache.canonicalize (hash, obj); - - if (! foundInFastBackend) - { - // If we have a fast back end, store it there for later. - // - if (m_fastBackend != nullptr) - m_fastBackend->store (obj); - - // Since this was a 'hard' fetch, we will log it. - // - WriteLog (lsTRACE, NodeObject) << "HOS: " << hash << " fetch: in db"; - } - } + foundInFastBackend = true; } - else + + // Are we still without an object? + // + if (obj == nullptr) { - // hash is known not to be in the database + // Yes so at last we will try the main database. + // + { + // Monitor this operation's load since it is expensive. + // + // VFALCO TODO Why is this an autoptr? Why can't it just be a plain old object? + // + // VFALCO NOTE Commented this out because it breaks the unit test! + // + //LoadEvent::autoptr event (getApp().getJobQueue ().getLoadEventAP (jtHO_READ, "HOS::retrieve")); + + obj = fetchInternal (m_backend, hash); + } + + } + + // Did we finally get something? + // + if (obj != nullptr) + { + // Yes it so canonicalize. This solves the problem where + // more than one thread has its own copy of the same object. + // + m_cache.canonicalize (hash, obj); + + if (! foundInFastBackend) + { + // If we have a fast back end, store it there for later. + // + if (m_fastBackend != nullptr) + m_fastBackend->store (obj); + + // Since this was a 'hard' fetch, we will log it. + // + WriteLog (lsTRACE, NodeObject) << "HOS: " << hash << " fetch: in db"; + } } } else @@ -192,7 +177,6 @@ public: m_fastBackend->store (object); } - m_negativeCache.del (hash); } } @@ -212,7 +196,6 @@ public: void sweep () { m_cache.sweep (); - m_negativeCache.sweep (); } int getWriteLoad () @@ -316,7 +299,6 @@ private: // VFALCO NOTE What are these things for? We need comments. TaggedCacheType m_cache; - KeyCache m_negativeCache; }; //------------------------------------------------------------------------------