shared_mutex: Add RAII locks #2196

dawmd · 2024-04-17T05:16:45Z

In this PR, we introduce RAII locks for seastar::shared_mutex, similar to seastar::semaphore_units and seastar::gate::holder.

include/seastar/core/shared_mutex.hh

bhalevy · 2024-04-17T06:32:29Z

include/seastar/core/shared_mutex.hh

+/// The class is NOT responsible for locking a \ref shared_mutex.
+/// \see get_unique_lock(shared_mutex&)
+/// \see shared_lock
+class unique_lock {


The counterpart of shared lock is typically an exclusive lock, not unique.

Let's stay consistent with:

/// Lock the \c shared_mutex for exclusive access /// /// \return a future that becomes ready when no access, shared or exclusive /// is granted to anyone. future<> lock() noexcept {

The C++ standard library has std::shared_lock and std::unique_lock, and I assume this is wheret his choice of names comes from. The questions I raised in my review was why can't we use std::shared_lock and std::unique_lock themselves, and not just their names.

include/seastar/core/shared_mutex.hh

tests/unit/locking_test.cc

bhalevy

see comments inline

xemul · 2024-04-17T07:45:35Z

I think it's worth having SEASTAR_SEMAPHORE_DEBUG analogue for this holder from the very beginning

nyh · 2024-04-17T09:51:50Z

Before I review the specific code, I want to ask whether we could use the standard C++ (C++14) classes std::shared_lock<seasatar::shared_mutex> and std::unique_lock<seastar::shared_mutex>, instead of implementing our own classes with exactly the same name (shared_lock and unique_lock) to hold the locks.

dawmd · 2024-04-17T14:59:21Z

Before I review the specific code, I want to ask whether we could use the standard C++ (C++14) classes std::shared_lock<seasatar::shared_mutex> and std::unique_lock<seastar::shared_mutex>, instead of implementing our own classes with exactly the same name (shared_lock and unique_lock) to hold the locks.

I don't think the APIs of std::unique_lock and std::shared_lock allow for that, at least not in a simple enough way to prefer that to implementing our own locks. But it's also quite likely I Just don't see how to do it

nyh · 2024-04-17T15:19:10Z

Before I review the specific code, I want to ask whether we could use the standard C++ (C++14) classes std::shared_lock<seasatar::shared_mutex> and std::unique_lock<seastar::shared_mutex>, instead of implementing our own classes with exactly the same name (shared_lock and unique_lock) to hold the locks.

I don't think the APIs of std::unique_lock and std::shared_lock allow for that, at least not in a simple enough way to prefer that to implementing our own locks. But it's also quite likely I Just don't see how to do it

std::shared_lock and std::unique_lock should work on any SharedLockable (see https://en.cppreference.com/w/cpp/named_req/SharedLockable). Doesn't our seastar::shared_mutex fit that constraint? If not, why not? What happens when you try?

dawmd · 2024-04-17T16:22:32Z

std::shared_lock and std::unique_lock should work on any SharedLockable (see https://en.cppreference.com/w/cpp/named_req/SharedLockable). Doesn't our seastar::shared_mutex fit that constraint? If not, why not? What happens when you try?

I don't think it satisfies that constraint. seastar::shared_mutex::{lock(), lock_shared()} doesn't block until the mutex can be obtained; it immediately returns a future and only when that future resolves can the mutex be obtained. The type thus only satisifes the Lockable requirement in my opinion.

I think it might be possible to somehow make it work, but I'm not knowledgeable enough to argue for or against the existence of a solution. Perhaps C++20's coroutines have somehow changed the situation?

Nonetheless, I attempted to make it work, but even this test

SEASTAR_THREAD_TEST_CASE(test_standard_unique_lock) {
    shared_mutex sm{};
    unsigned counter = 0;

    parallel_for_each(boost::irange(0, 10), [&] (int idx) -> future<> {
        return async([&] {
            const auto lock = std::unique_lock(sm);
            BOOST_REQUIRE_EQUAL(counter, 0u);
            ++counter;
            sleep(1ms).get();
            --counter;
            BOOST_REQUIRE_EQUAL(counter, 0u);
        });
    }).get();
}

fails with the following repeated error:

[...]/src/seastar/tests/unit/locking_test.cc(473): fatal error: in "test_standard_unique_lock": critical check counter == 0u has failed [1 != 0]

We could defer the locking when constructing the lock and only then attempt to do it, but std::shared_lock<Mutex>::{lock(), shared_lock()} returns a void, so we lose any access to the underlying future we should await.

nyh · 2024-04-21T12:23:48Z

@dawmd yes, surely

const auto lock = std::unique_lock(sm);

would not work, but does something like this work?

co_await sm.lock();
return std::unique_lock(sm, std::adopt_lock);

In this code we only use the std::unique_lock() to wrap an already taken lock - not to actually take it.
You'll still have a new function to get the lock - but it will return an std::unique_lock object and not a seastar::unique_lock.

If it doesn't work I'll give up, and review your code :-)

dawmd · 2024-04-21T16:48:25Z

@dawmd yes, surely
const auto lock = std::unique_lock(sm);
would not work, but does something like this work?
co_await sm.lock();
return std::unique_lock(sm, std::adopt_lock);
In this code we only use the std::unique_lock() to wrap an already taken lock - not to actually take it. You'll still have a new function to get the lock - but it will return an std::unique_lock object and not a seastar::unique_lock.

If it doesn't work I'll give up, and review your code :-)

Thank you. I didn't understand what you meant at first, but you're right.

Version 2:

Got rid of the redundant lock classes. Switched to std::unique_lock and std::shared_lock instead.
The name of the function – get_unique_lock – was preserved since we return an std::unique_lock now.

bhalevy · 2024-04-21T19:22:42Z

include/seastar/core/shared_mutex.hh

+    co_await mutex.lock_shared();
+
+    try {
+        co_return std::shared_lock<shared_mutex>{mutex, std::adopt_lock_t{}};


std::shared_lock is copyable, calling the murex.lock_shared, but in seastar case, this returns a future. We should prevent this.

It seems it's actually not. Fortunately the standard specifies that both the copy constructor and the copy assignment operator are both deleted (link).

I think constraining the API of the locks so that the user cannot call e.g. std::shared_lock<Mutex>::try_lock_for() might be worth considering, though.

yes, sorry, I confused it with another ctor.

dawmd · 2024-04-22T13:45:01Z

Version 3:

I forgot to coroutinize the tests I was adding. It's done now.
Some of the new tests were redundant as they were testing the same scenarios others already did. They are removed in this version.
I updated the commit message.

bhalevy · 2024-04-22T13:56:58Z

include/seastar/core/shared_mutex.hh

+///
+/// The caller is responsible for keeping the corresponding \ref shared_mutex object
+/// alive at least until the returned lock is destroyed.
+inline future<std::shared_lock<shared_mutex>> get_shared_lock(shared_mutex& mutex) {


We should consider making the mutex type a template parameter so it could be extended/overridden, even for testing.

dawmd · 2024-04-22T14:12:02Z

Version 4:

CI failures fixed: when the macro SEASTAR_ENABLE_ALLOC_FAILURE_INJECTION was resolved to 0, some of our functions didn't return or await anything causing compilation errors.

dawmd · 2024-04-24T10:42:53Z

Version 5:

Turned the functions into function templates taking the mutex type as their parameter.

piodul · 2024-05-02T16:05:56Z

@nyh, would you like to review? You said that you would like to do so.

nyh

Looks good, and almost ready for merge, but I do have a few comments and questions I'd like you to please consider.

nyh · 2024-05-02T18:26:28Z

include/seastar/core/shared_mutex.hh

+///
+/// The caller is responsible for keeping the corresponding Mutex object
+/// alive at least until the returned lock is destroyed.
+template <typename Mutex>


nitpick: I'm not sure the name "Mutex" really represents the type of object this is. It needs to have exactly the right methods lock_shared(), unlock_shared(). Would have been nice to create a concept like https://en.cppreference.com/w/cpp/named_req/SharedLockable (just with a different return type for lock_shared()) and use that concept.
But it's a nitpick, not critical.

That's a good idea. I'll add the concepts

include/seastar/core/shared_mutex.hh

tests/unit/locking_test.cc

nyh · 2024-05-02T18:37:31Z

tests/unit/locking_test.cc

+    }));
+
+    BOOST_REQUIRE_EQUAL(counter, 0u);
+    BOOST_REQUIRE_NE(max, 0u);


I don't think this last check checks anything - obviously max can't be 0 because it will always be at least 1 (you increment counter, so it's at least 1, and then set max to at least that).

I'm not sure you really needed to check all this parallelization stuff, I think test_shared_mutex_locks above already checks the same thing more simply (it tries to take the same lock 3 times, and it succeeds).

Can you please answer this?
It's not a critical issue (who cares if a test checks a silly condition that can never be false), but it is a hint that maybe you missed the fact that the test isn't testing what you think it checks - so maybe you forgot to test what you really wanted to test, or something. So please just have a look.

I'm sorry for the late response. Most of the tests I added are analogues of the existing tests with using locks and coroutines being the only difference. I think you're right it's not necessary to check those things again. You're definitely right that comparing max against 0 is redundant. It would make more sense if Seastar were preemptive. I'll remove the test.

nyh · 2024-05-02T18:43:12Z

tests/unit/locking_test.cc

+    BOOST_REQUIRE_THROW(co_await std::invoke([&] () -> future<> {
+        const auto slock = co_await get_shared_lock(sm);
+        BOOST_REQUIRE(false);
+    }), std::bad_alloc);


Can you please help me understand what we're trying to test here? That the coroutine functions need to allocate and if they can't, they fail before even starting? Isn't this obvious for all coroutine functions? Is this really what you hoped to test here?

As in #2196 (comment), this test is an analogue of the existing one, adjusted to using locks and coroutinised. The way I understand the logic here is we want to check what happens in case of an allocation failure: we enter a coroutine and await a lock. However, the lock will never be returned because the allocation of the coroutine awaiting it will fail, throwing an std::bad_alloc (alternatively, the underlying allocation of a promise performed by seastar::shared_mutex will fail. Either way, there should be an allocation failure). However, now that I look at it, it seems overly complex and probably unnecessary; chances are I also don't understand it correctly. I can remove the test.

nyh · 2024-05-02T18:44:24Z

tests/unit/locking_test.cc

+    seastar::memory::local_failure_injector().cancel();
+#endif // SEASTAR_ENABLE_ALLOC_FAILURE_INJECTION
+
+    // If the macro resolves to 0, we need this to prevent compilation errors


since you used ifdef and not if, I assume you mean if SEASTAR_ENABLE_ALLOC_FAILURE_INJECTION is not defined - not if it "resolves to 0".

nyh · 2024-05-02T18:47:06Z

tests/unit/locking_test.cc

+    try {
+        const auto slock = co_await get_shared_lock(sm);
+        if (expected == 42) {
+            throw expected_exception(expected);


In test_shared_mutex_failed_func_locks above you already tested that an exception correctly unlocks the mutex. This seems to test the same thing, but not as well (you didn't check that sm got unlocked!). So why do we need this test? Why does this test's name have the strings "return_nothrow_move" - what does this refer to?

Good catch. I forgot the rename the test. It was originally doing something else (and as most of the other tests I added, it was based on the existing ones). But you're right that it's unnecessary. I'll remove it.

nyh

The code looks good, and I resolved some of my old comments, but I still have some unanswered questions about the test, so can you please answer them? Thanks.

include/seastar/core/shared_mutex.hh

tests/unit/locking_test.cc

nyh · 2024-05-19T09:38:48Z

tests/unit/locking_test.cc

+    }));
+
+    BOOST_REQUIRE_EQUAL(counter, 0u);
+    BOOST_REQUIRE_NE(max, 0u);


Can you please answer this?
It's not a critical issue (who cares if a test checks a silly condition that can never be false), but it is a hint that maybe you missed the fact that the test isn't testing what you think it checks - so maybe you forgot to test what you really wanted to test, or something. So please just have a look.

Most of the tests this commit adds are existing ones adjusted to using RAII locks obtained by calls to `seastar::get_shared_lock()` and `seastar::get_unique_lock()`

dawmd · 2024-05-19T16:49:13Z

Version 2:

Rebased the PR on top of the current master branch (because of C-ares errors regarding deprecated functions)
Added concepts imposing constraints on the types accepted by get_shared_lock() and get_unique_lock(). Adjusted the comments accordingly
Renamed the test test_shared_mutex_failed_func_locks to test_shared_mutex_exception_locks to better express what it verifies
Removed the tests:
- test_shared_mutex_shared_locks
- test_shared_mutex_failed_lock_locks
- test_shared_mutex_failed_lock_mixed_locks
- test_with_shared_typed_return_nothrow_move_func_locks

dawmd requested a review from bhalevy April 17, 2024 05:16