feat: Add mutex wrapper from clio

[kuznetsss]

High Level Overview of Change

This PR adds a mutex wrapper copied from clio. This wrapper makes a mutex attached to the data it protects which improves safety and readability.

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• [] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[Copilot]

Pull request overview

This PR adds a new mutex+data wrapper (ported from clio) intended to bundle protected data with its mutex and provide an RAII lock object that grants scoped access to the data.

Changes:

• Add xrpl::Mutex<T, MutexType> container that owns T and a mutex.
• Add xrpl::Lock<T, LockType, MutexType> to hold the lock and provide *, ->, and get() accessors.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (1ee0567) to head (c3db38a).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6447   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          861     862    +1     
  Lines        67857   67876   +19     
  Branches      7553    7556    +3     
=======================================
+ Hits         54169   54185   +16     
- Misses       13688   13691    +3     

Files with missing lines	Coverage Δ
include/xrpl/basics/Mutex.hpp	100.0% <100.0%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vlntb]

LGTM. Let's resolve the question about the unit test, and I'm happy to approve.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[godexsoft]

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ximinez]

This is not a "chore". It is a new feature. Just because it's not user-facing doesn't mean it's not new.

[ximinez]

Does this leave the data vulnerable to modification outside of lock?

[kuznetsss]

Do you mean that it is possible to get reference to the internal lock, unlock it and change underlying data?

It is not perfectly safe, but having this conversion we are able to use lock with condition variables. We use it in clio only in one place:

       auto lock = data_.lock<std::unique_lock>();
       cv_.wait(lock, [&] { return stopping_ or not lock->forwardLoadQueue.empty(); });

[ximinez]

Do you mean that it is possible to get reference to the internal lock, unlock it and change underlying data?

Yeah, basically.

Like in the test here, you could write

    std::unique_lock<std::mutex>& ul = lock;
    ul.unlock();
    ul.lock(); 
    EXPECT_EQ(*lock, 2);
    ul.unlock();
	*lock = 4;
    ul.lock(); 
    EXPECT_EQ(*lock, 4);

And I'm assuming it would "pass", even though it's clearly not what we want.

What do you think about using some template magic to check owns_lock if the LockType class defines one, and throwing if it's false everywhere you return data? It's probably overkill, and it's easy to defeat by grabbing a reference before releasing the lock, but it would catch accidental usages.

It is not perfectly safe, but having this conversion we are able to use lock with condition variables. We use it in clio only in one place:

       auto lock = data_.lock<std::unique_lock>();
       cv_.wait(lock, [&] { return stopping_ or not lock->forwardLoadQueue.empty(); });

That is an excellent use case, though!

[kuznetsss]

I think adding a check for owns_lock will slightly impact performance and as you said, it will not completely prevent unsafe usage of this class.
The best solution probably would be to remove the conversion operator and add a wrap for condition variable (maybe not in this PR but when it will be needed). What do you think?

[ximinez]

I think adding a check for owns_lock will slightly impact performance and as you said, it will not completely prevent unsafe usage of this class. The best solution probably would be to remove the conversion operator and add a wrap for condition variable (maybe not in this PR but when it will be needed). What do you think?

That's a great idea, and I'm fine with doing it in a follow-up PR.