From 495e2f8247e97621f69ad96a8f5b9b7ba7c75920 Mon Sep 17 00:00:00 2001
From: Ed Hennis <ed@ripple.com>
Date: Wed, 7 Aug 2024 18:08:03 -0400
Subject: [PATCH] refactor: Rework database lock management

* Follow-up to #4989
---
 src/xrpld/nodestore/DatabaseRotating.h           |  4 ++++
 .../nodestore/detail/DatabaseRotatingImp.cpp     | 16 +++++++++++++++-
 src/xrpld/nodestore/detail/DatabaseRotatingImp.h | 10 +++-------
 3 files changed, 22 insertions(+), 8 deletions(-)

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<std::unique_ptr<NodeStore::Backend>(
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::unique_ptr<NodeStore::Backend>(
         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 5183aa1e2e4..d857e3dc2d6 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<std::unique_ptr<NodeStore::Backend>(
@@ -82,13 +84,7 @@ class DatabaseRotatingImp : public DatabaseRotating
 private:
     std::shared_ptr<Backend> writableBackend_;
     std::shared_ptr<Backend> 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<NodeObject>
     fetchNodeObject(