Skip to content

Commit

Permalink
refactor: Rework database lock management
Browse files Browse the repository at this point in the history
* Follow-up to XRPLF#4989
  • Loading branch information
ximinez committed Aug 7, 2024
1 parent c19a88f commit 08f9e01
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 8 deletions.
4 changes: 4 additions & 0 deletions src/xrpld/nodestore/DatabaseRotating.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand Down
16 changes: 15 additions & 1 deletion src/xrpld/nodestore/detail/DatabaseRotatingImp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 3 additions & 7 deletions src/xrpld/nodestore/detail/DatabaseRotatingImp.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand Down Expand Up @@ -81,13 +83,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(
Expand Down

0 comments on commit 08f9e01

Please sign in to comment.