diff --git a/src/xrpld/nodestore/DatabaseRotating.h b/src/xrpld/nodestore/DatabaseRotating.h index 10f575c4662..d923def5f3e 100644 --- a/src/xrpld/nodestore/DatabaseRotating.h +++ b/src/xrpld/nodestore/DatabaseRotating.h @@ -45,6 +45,10 @@ class DatabaseRotating : public Database /** Rotates the backends. @param f A function executed before the rotation and under the same lock + + This function only has one caller in SHAMapStoreImp::run. The locking + for this function will need to be rethought another caller is ever + added. */ virtual void rotateWithLock(std::function( diff --git a/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp b/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp index 58cc3599dc6..8b9a311d3ac 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp @@ -45,9 +45,23 @@ DatabaseRotatingImp::rotateWithLock( std::function( std::string const& writableBackendName)> const& f) { + auto const writableBackend = [&] { + std::lock_guard lock(mutex_); + return writableBackend_; + }(); + + auto newBackend = f(writableBackend->getName()); + std::lock_guard lock(mutex_); - auto newBackend = f(writableBackend_->getName()); + // This function only has one caller, which is syncronous, and this is the + // only place other than the ctor where writableBackend_ can change. Even + // without a lock, it should be impossible for writableBackend_ to have + // changed. + assert(writableBackend_ == writableBackend); + if (writableBackend_ != writableBackend) + LogicError("Backend changed in the middle of a rotation"); + archiveBackend_->setDeletePath(); archiveBackend_ = std::move(writableBackend_); writableBackend_ = std::move(newBackend); diff --git a/src/xrpld/nodestore/detail/DatabaseRotatingImp.h b/src/xrpld/nodestore/detail/DatabaseRotatingImp.h index 0c17dc59ceb..76b083e3f7d 100644 --- a/src/xrpld/nodestore/detail/DatabaseRotatingImp.h +++ b/src/xrpld/nodestore/detail/DatabaseRotatingImp.h @@ -48,6 +48,8 @@ class DatabaseRotatingImp : public DatabaseRotating stop(); } + // This function only has one caller in SHAMapStoreImp::run. The locking for + // this function will need to be rethought another caller is ever added. void rotateWithLock( std::function( @@ -81,13 +83,7 @@ class DatabaseRotatingImp : public DatabaseRotating 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(