Design review

This is not a bad idea, but I question the need. Are there really any situations where you want to repeatedly lock and unlock multiple lockables in lockstep?

Note that I stress repeatedly, because if you just want to lock/unlock a set of lockables once, std::scoped_lock does that job. Even when you do want to lock a bunch of things repeatedly, using multiple scopes with scoped_lock probably makes more sense than hanging on to a potentially non-locked lock.

Indeed, even std::unique_lock with a single lockable is generally unwise. Almost every use of unique_lock would be better off using scoped_lock. The single use case I can think of for unique_lock is when using condition variables, because the point there is that you want to unlock to back off and let things happen then re-lock and see if the condition has been triggered.

In fact, I teach to never use unique_lock… except for condition variable stuff.

The only other use cases for unique_lock are:

• when you want to move locks around… which is generally unwise (and even more unwise with a lock that’s locking multiple things); or
• when you want to lock multiple lockables, using the defer_lock strategy… which is done better in just about every situation using scoped_lock.

To be clear, I’m not dumping on the idea. It’s just that the way I see it, other than the condition variable case, any situation that is complicated enough to require unique_lock is already a mess. I don’t see how it could be improved by a multiple unique_lock.

But that’s just from my experience. If you have an actual use case where this kind of thing might be useful, then fine. And there is one interesting feature of unique_multilock… although with a caveat.

try_lock_for/until()

So the real meat of the unique_multilock API is these functions:

lock();
try_lock();
try_lock_for(duration);
try_lock_until(time_point);

The other stuff is all just observers or things like unlock() which really just follow from the above API.

Now the first two functions are basically just:

std::lock(...);
std::try_lock(...);

… using the lockables associated with the unique_multilock.

But the other two functions are novel. And, dare I say, potentially very useful!

However, there are two problems.

The first is that these functions are so useful, they should not be in the class. Just as unique_multiple<Ms...>::lock() is really just a thin wrapper around std::lock(ms...) and unique_multiple<Ms...>::try_lock() is really just a thin wrapper around std::try_lock(ms...), unique_multiple<Ms...>::try_lock_for(duration) and unique_multiple<Ms...>::try_lock_until(time_point) should really just be thin wrappers around general try_lock_for(duration, ms...) and try_lock_until(time_point, ms...) functions.

Those free functions are more generally useful than the member functions could ever be. Indeed, just as with std::lock(...) and std::try_lock(...), the member functions could be implemented in terms of the free functions.

So whether you want unique_multilock or not, it would probably make sense to pull those functions out of the class, and then, if desired, implementing the related member functions in terms of them.

But that brings us to the second problem.

So while there are standard specifications for how lock(lockables...) and try_lock(lockables...) should behave, there are no standard specification for how a hypothetical try_lock_for(duration, lockables...) or try_lock_until(time_point, lockables...) should be behave. That means that, theoretically, whatever you chose to make them do is “correct”.

However, while that may be theoretically true, I think there are practical reasons why some behaviours make more sense than others.

Consider lock(lockables...) and try_lock(lockables...). These functions are closely related, yet completely different even from a conceptual perspective.

• lock(lockables...): “Do everything you can to lock all the lockables, including backing off and retrying in a different order if necessary. Keep trying until forever.”
• try_lock(lockables...): “Make an attempt to lock all the lockables, in given order. Try only once.”

Now, when considering the behaviour of try_lock_until(time_point, lockables...), does it make more sense to base it on the behaviour of lock() or try_lock()? In other words, should it be:

• “Do everything you can to lock all the lockables, including backing off and retrying in a different order if necessary. Keep trying until time_point.” … or…
• “Make an attempt to lock all the lockables, in given order. Repeat until time_point.”

See what I’m getting at? Your current approach is the latter. But… does that feel correct?

In my mind, a try_lock_until() function shouldn’t just repeat std::try_lock() over and over until it either succeeds or time runs out. Nor should it just call .try_lock_until() on each lockable until one fails. It should do whatever it takes, even backing off and retrying in a different order. In other words, it should be std::lock() with a time limit.

It might look something like this:

auto try_lock_until(std::chrono::time_point<Clock, Duration> const& tp, lockable auto&... l)
{
    // For simplicity, assuming all the lockables are the same type,
    // and not caring about the possibility of zero lockables.
    auto lockables = std::array{std::addressof(l)...};

    do {
        // Lock the first lockable.
        auto lock_1 = std::unique_lock{*(lockables[0]), std::defer_lock};
        if (not lock_1.try_lock_until(tp));
        {
            // We couldn't even get one lock before time ran out!
            return false;
        }

        // Try to lock the remaining lockables.
        auto other_locks = std::array<std::unique_lock, lockables.size() - 1>{};
        auto success = true; // we live in hope!
        for (auto&& l : std::views::zip(lockables | std::views::drop(1), other_locks))
        {
            std::get<1>(l) = std::unique_lock{*std::get<0>(l), std::try_to_lock};
            if (not std::get<1>(l))
            {
                success = false;
                break;
            }
        }

        if (success)
        {
            // All lockables have been locked!
            lock_1.release();
            std::ranges::for_each(other_locks, [](auto&& l) { l.release(); });
            return true;
        }
        else
        {
            // Rotate the lockables, and (maybe) try again.
            std::ranges::rotate(lockables, std::ranges::next(std::ranges::begin(lockables)));
        }
    } while (tp < Clock::now());

    return false;
}

… and similar for try_lock_for() (or delegate). And of course, the member function try_lock_until() can now basically be based on this.

And this seems like a more realistic idea of what you’d expect from try_lock_until. It doesn’t restrict itself to repeatedly trying to lock in the same order and immediately giving up on the first failure—even if time remains—to unlock everything and try again. It just does whatever it can until success or time running out.

So while I’m a little cool on the idea of unique_multilock, I’m quite keen on try_lock_until(time_point, lockables...) and try_lock_for(duration, lockables...) free functions. And since unique_multilock is basically free once you have those… why not, I guess?

Questions

“Is it correct when it comes to the handling of the mutexes?”

That depends on what you mean by “correct”.

If you mean it doesn’t trigger UB or have race conditions or such, then it seems correct to me.

If you mean “correct” in the sense “it satisfies the contract”… well that depends on what the contract is. That is what that big section about try_lock_until(time_point)/try_lock_for(duration) was about. Is the contract for those functions to keep trying a single lock strategy over and over until time runs out… or is it to try any lock strategy that might work, until time runs out?

“Implementation improvements? Could the spinning in try_lock_until be done more efficiently somehow for example.”

I can’t see much that could be improved, except in the implementation of try_lock_until().

Yes, you do have a busy-wait loop in try_lock_until() with the current implementation… which is part of the reason I think the current implementation is misguided. I don’t think it makes a lot of sense to repeatedly try locking the lockables in the same order, immediately giving up when one won’t immediately lock, then immediately trying the exact same thing again over and over. That’s both wasteful, and pretty close to the proverbial “definition of insanity”.

The very rough implementation I made up above does not busy wait (well… not unless the lockable’s .try_lock_until() busy-waits). It uses a timed wait for the first lockable, then immediately tries all the others and bails if any don’t immediately lock. If any don’t lock, unlock everything, shuffle the order, and retry. That avoids both busy-waiting, and potential lock order problems.

That is not the only strategy. Perhaps every one of the locks should be try_lock_until()-ed. The problem with that strategy is you get exactly one attempt; if you don’t get all the locks in the time limit, the time limit will be up, so you can’t retry in a different order.

And there is good reason to want to try locking the locks in a different order. It may be that locking one lock prevents one of the other locks from being unlocked elsewhere, even though it theoretically could be. By retrying in a different order, you increase the odds of a successful lock. That’s how std::lock() works. It’s just unfortunate that it doesn’t have a timeout.

“API: try_lock returns int -1 for success just like std::try_lock. It's a bit ugly.”

Eh.

The thing about std::try_lock() is that it is a multi-faceted function. It is not just a fail-fast std::lock(). Yes, sure, that is one of the ways it could be used. But there are others.

std::try_lock() is very specific about the fact that it tries to lock all the lockables in order. That is important: in order. That means there is more information you get from the success/failure if std::try_lock() beyond merely just “they locked or did not lock”.

Among other things, you could use std::try_lock() to test:

• if any of the given locks is already locked
• which of the given locks is already locked
• that the given locks can be locked in that order
• … etc.

Because the function is so multifaceted, it is not clear what a “desired” result looks like, or what an “error” result is.

That is why I would say optional<size_t> is not really a good return type for this function. If the function’s specific purpose was “find the first lockable that can’t be locked (without waiting, of course), if any”, then, sure, std::optional<std::size_t> would be the perfect result type: it is pretty much exactly what you want: (the index of) the first lockable that couldn’t be locked… if any.

But how often have you used std::try_lock() to find a lockable that won’t lock? I’ve never. I’ve only used it to try to lock ’em all, and if any won’t, quickly take advantage of that information to do something else. For that purpose, the ideal return type is bool. I don’t care which lockable couldn’t lock.

What about std::expected? Well, that would be the perfect return type if the purpose of the function was “lock all of these, and if you can’t there’s a problem/error” (as opposed to the previous case, which was “lock ’em all, and if you can’t, that’s fine (not a problem or error), just let me know”)… which is yet another perfectly valid use of the function. For that usage, the ideal return type might be std::expected<void, std::size_t>.

Because std::try_lock() is such a low-level and multi-faceted operation, what you should do is make a higher-level wrapper function for whichever of the specific things you actually mean (or all of them), and that function should return the logical result type for that conceptual operation.

But for a generic, low-level, context-free operation like naked try_lock()… what’s wrong with a result as simple as “the failing index or -1”?

Okay, okay, I hate magic numbers, so let’s get crazy and imagine what the perfect result type for try_lock() should be.

No, it should definitely not be optional<std::size_t>. Or expected<void, std::size_t>. Not only because those types represent concepts that may not be relevant to the particular use of the function, but also because those types are a lot more expensive than a bare int. Both require at least an int (or std::size_t) and a boolean-like discriminator, and both have costs to access the int due to checks to make sure there is an int present.

Let’s start with the basics:

struct try_lock_result_t
{
    int index = -1;
};

This seems like a step backward from a bare int… but then we can do:

struct try_lock_result_t
{
    int index = -1;

    constexpr auto all_locked() const noexcept { return index == -1; }
    constexpr explicit operator bool() const noexcept { return all_locked(); }
};

And voilà, no more magic numbers:

// I want to confirm that all locked:
if (try_lock(lockables...).all_locked()) ...

// I want to do something with the one that didn't lock:
if (auto const result = try_lock(lockables...); not result.all_locked())
    std::println("the one that didn't lock was {}", result.index);

// Or perhaps:
try_lock(lockables...) or do_alternative_work();

And there is zero additional cost over a bare int.

I suppose you could also add a set of comparisons with std::size_t, so you could do if (result == 2) rather than if (result.index == 2).

The main point of this bespoke result type is that it doesn’t privilege any particular usage of the function. Whatever you mean by using try_lock(), the result reflects that.

Exception handling? Did I miss anything?

Looks fine to me.

Code review

template <class T>
concept Lockable = BasicLockable<T> && requires(T t) {
    { t.try_lock() } -> std::same_as<bool>;
};

std::same_as<bool> seems a bit restrictive. Surely all you need here is convertible_to<bool>, or std::equality_comparable_with<bool>. (Ideally, what you want is boolean-testable, but that’s expo-only.)

template <class T>
concept TimedLockable = Lockable<T> && requires(T t) {
    {  // using an unusal ratio to avoid false positives for hardcoded duration
       // types
        t.try_lock_for(std::chrono::duration<std::int_least32_t,
                                             std::ratio<1, 999999999>>{})
    } -> std::same_as<bool>;

There is no point worrying about shenanigans when writing a concept. Remember that concepts are supposed to not only be testing syntactic conformance, but semantic conformance as well. I mean, just consider:

template <class T>
concept BasicLockable = requires(T t) {
    t.lock();
    t.unlock();
};

I could make a type that “satisfies” this concept where t.lock() puts a document on the printer spool and t.unlock() erases the hard drive. Or I could make a type where t.lock() does indeed lock something, and t.unlock() unlocks it… but you have to call t.lock() three times for every t.unlock().

So there’s no need to get too “clever”. You could just as well just check a few basic types:

template <class T>
concept TimedLockable = Lockable<T> && requires(T t) {
    { t.try_lock_for(std::chrono::nanoseconds{}) } -> std::convertible_to<bool>;
    { t.try_lock_for(std::chrono::weeks{}) } -> std::convertible_to<bool>;

    { t.try_lock_until(std::chrono::steady_clock::now()) } -> std::convertible_to<bool>;
    { t.try_lock_until(std::chrono::high_resolution_clock::now()) } -> std::convertible_to<bool>;
    { t.try_lock_until(std::chrono::local_seconds{}) } -> std::convertible_to<bool>;
};

If anything satisfies that much of the concept while failing to account for any clock or time point type… well… I mean… that’s hardly a problem with the concept.

However, I’ll point out later that all of this is probably a non-issue.

    { t.try_lock_until(std::chrono::steady_clock{}) } -> std::same_as<bool>;

🤨

template <class... Ms>
    requires((... && Lockable<Ms>) ||
             (sizeof...(Ms) == 1 && (... && BasicLockable<Ms>)))

I’m guessing you lifted these requirements from std::scoped_lock. Well, while they make sense there, they make no sense here.

These are the requirements for a function that tries to lock multiple lockables with a deadlock-avoidance strategy. They are, in effect, the requirements for std::lock(), except that that function doesn’t take less than 2 arguments. They are the requirements for std::scoped_lock because that can take less than 2 arguments, and when it does take 1, it can lock without a deadlock-avoidance strategy.

But unique_lock ≠ scoped_lock, and not just because it doesn’t take multiple lockables. Even in just the case of a single lockable, unique_lock ≠ scoped_lock. unique_lock has facilities and capabilities that scoped_lock can’t even dream about.

For example, have you considered that you might want to unique_multilock multiple “things” that can’t even be locked?

What? Did Indi just say something crazy?

Observe:

// An emergency hatch is *created* "locked", but blows open when there
// is an emergency, and can never be "re-locked".
struct emergency_hatch
{
    emergency_hatch();
    void unlock();
};

auto hatch_1 = emergency_hatch{};
auto hatch_2 = emergency_hatch{};
auto hatch_3 = emergency_hatch{};

// We're about to do a dangerous operation!
{
    auto hatches = unique_multilock{std::adopt_lock, hatch_1, hatch_2, hatch_3};

    do_something_dangerous_that_might_throw();

    // We're safe! No need to be ready to blow the emergency hatches anymore.
    hatches.release();
}

You see? We never call .lock() or .try_lock() or anything related. We might call .unlock(). But we need the .release() mechanism (which std::scoped_lock does not have) when there is no emergency.

If you require Lockable for all the lockables, then the above can’t work. Hell, even if you require just BasicLockable, it still won’t work.

Does that make sense? Is the example I provided above wrong? The type doesn’t meet the requirements. So is it wrong to use unique_multilock like this? Or are the requirements wrong?

I think the only sensible requirement for unique_multilock is actually UNlockable. As in:

template<typename T>
concept unlockable = requires(T& t) { t.unlock(); };

… because, as I illustrated above, you don’t need the types to be lockable to constructor or use them with unique_multilock. But you do need them to be unlockable, or the destructor can’t compile (so you can’t create the unique_multilock in the first place). Even if you don’t actually unlock anything in the destructor—the example above will not if there is no emergency—you need to be able to.

That actually makes the requires clause much simpler:

template<typename... Ts>
    requires (unlockable<Ts> && ...)
class unique_multilock
{

… or even just:

template<unlockable... Ts>
class unique_multilock
{

I can’t think of any other requirements on the template parameters.

    unique_multilock(const unique_multilock&) = delete;
    unique_multilock(unique_multilock&& other) noexcept
        : m_ms(std::exchange(other.m_ms, std::tuple<Ms*...>{})),
          m_locked(std::exchange(other.m_locked, false)) {}
    unique_multilock& operator=(const unique_multilock&) = delete;
    unique_multilock& operator=(unique_multilock&& other) noexcept {
        unique_multilock(std::move(other)).swap(*this);
        return *this;
    }
    ~unique_multilock() {
        if (m_locked) unlock();
    }

Does all this really need to be that squished together? Even if I were printing this and worried about wasting paper, I would spread this out more. It’s nigh unreadable, and it’s hard to spot that there are some places where multiple things are happening on a single line (like in the move constructor, where the final {} is very well hidden).

    unique_multilock(Ms&... ms) : m_ms(std::addressof(ms)...) { lock(); }

This constructor needs to be explicit. Indeed, this is true for all of the non-copy/move constructors. Right now, the lock is implicitly convertible from a single lockable argument. And in the case of the other constructors, you have locks that implicitly convert from things like time points and std::defer_lock.

Note that you really only need the constructor to be explicit when it is being used with a single argument. It’s not really a problem with multiple arguments. So you could do:

    explicit(sizeof...(Ms) == 1) unique_multilock(Ms&... ms)

And similarly for the other constructors:

    explicit(sizeof...(Ms) == 0) unique_multilock(std::defer_lock_t, Ms&... ms) noexcept

Now, there is a very deep and important design decision you have to make. unique_multilock, like std::scoped_lock, allows for zero lockables… even though, while harmless, it doesn’t really make a lot of sense to lock “nothing”. The reason why std::scoped_lock allows it is for ease of use in generic contexts; it always “works”, no matter what the number of template arguments is. std::scoped_lock would be much more of a pain to use in generic contexts if you had to check for and special-case the zero-argument case.

However, some people consider this to be a design flaw, and even recommend against using std::scoped_lock for single-argument cases (and they say instead you should use std::lock_guard).

Why? Well consider the example below:

{
    std::scoped_lock lock;

    // ...
}

Whoops! We have locked nothing! We made a typo! We meant to do:

{
    std::scoped_lock lock{mutex};
//-----------------------^^^^^^^

    // ...
}

But the former code compiles without complaint, and “works”. You may not notice for a long, long time that you never actually lock the mutex. Race conditions often only trigger visible problems rarely or sporadically. This is a nightmare bug.

This is not a problem with std::lock_guard:

{
    std::lock_guard lock;   // compile error!
                            //
                            // std::lock_guard has no default constructor

    // ...
}

But it is a problem with unique_multilock (and std::unique_lock, coincidentally), because it does have a “default constructor”. That is, when you do unique_multilock{}, you are using the locking constructor with a zero-length template parameter pack.

If you think this is a problem you could forbid empty template argument lists.

(Full disclosure: I am not one of those people that thinks any of this is a problem. From where I stand, the problem in the “bad” std::scoped_lock example above is not with std::scoped_lock, but rather because of not using the “always auto” (formerly “almost always auto”) initialization style. In other words, if you always wrote auto lock = std::scoped_lock{...};, you can’t possibly forget the mutex. auto lock = std::scoped_lock; will simply not compile, and if you write auto lock = std::scoped_lock{};… well, I mean, that’s not a case of simply “forgetting”, because you had to explicitly type the empty braces to say you wanted no arguments.)

(Another solution that I’ve considered proposing is another constructor for std::scoped_lock with a tag, that forbids the empty pack case, like so scoped_lock(with_lockables_t, Ts&...) requires (sizeof...(Ts) > 0). Then if you are worried about the “typo”, you would always write scoped_lock lock{with_lockables, ...};, and if the ... is empty, you get a compile error, and lock_guard now becomes completely obsolete. But, frankly, I don’t take this “concern” seriously enough to care that much. Especially when simply using a better coding style fixes it automatically.)

Back to the constructor at hand:

    unique_multilock(Ms&... ms) : m_ms(std::addressof(ms)...) { lock(); }

Now this constructor needs the requires clause that is currently applied to the whole class. So does the member .lock() function, by the by. This constructor requires that all the arguments are at least basic-lockable, and to do deadlock-avoidance for multiple arguments, it needs try_lock() support.

    unique_multilock(std::adopt_lock_t, Ms&... ms) noexcept
        : m_ms(std::addressof(ms)...), m_locked(true) {}

I’m not sure this constructor should be noexcept.

noexcept does not mean “I don’t throw any exceptions”. It means “this operation cannot possibly fail”. If you argue that adopting a bunch of locks cannot possibly fail, then I have to counter with… but what if they are not already locked?

If the locks are not already locked, and you call unique_multilock{adopt_lock, ...}, then the lock object is immediately in a broken state. Unless you intervene either by calling .release(), or you actually lock those lockables externally to the class, then when the lock gets cleaned up… boom.

You may wonder why I didn’t bring this up as a problem with the defer_lock case: what if you do unique_multilock{defer_lock, ...} with a bunch of lockables that are already locked.

Well, my answer is: 🤷🏽.

Unlike the adopt_lock case, in the defer_lock case, the lock isn’t actually broken right off the bat. You would have to call .lock() or similar to actually break it… and that is where the breakage would be occurring, which is why those functions cannot be noexcept. (Didja notice that neither std::try_lock() nor std::mutex::try_lock() is marked noexcept, even though the standard promises the latter does not throw? This is why.)

Put another way:

• If you do unique_multilock{adopt_lock, ...} with non-locked objects, the lock is immediately broken; you have to take extra steps to un-break it.
• If you do unique_multilock{defer_lock, ...} with already-locked objects, the lock is fine… but it will break if you take certain extra steps.

That is why I think the adopt_lock constructor should not be marked noexcept, but it’s fine if the defer_lock constructor is.

    template <class Rep, class Period>
        requires(... && Lockable<Ms>)  // TimedLockable?
    unique_multilock(const std::chrono::duration<Rep, Period>& dur, Ms&... ms)
        : m_ms(std::addressof(ms)...), m_locked(try_lock_for(dur) == -1) {}

    template <class Clock, class Duration>
        requires(... && Lockable<Ms>)  // TimedLockable?
    unique_multilock(const std::chrono::time_point<Clock, Duration>& tp,
                     Ms&... ms)
        : m_ms(std::addressof(ms)...), m_locked(try_lock_until(tp) == -1) {}

Let’s think about the requirements on the lockables for these constructors.

Presumably the class requirements handle the unlockable constraint, so all we need to worry about is the locking side of things. At bare minimum, we want the lockables for the first constructor to satisfy:

template<typename T, typename Rep, typename Period>
concept ??? =
    requires(T& t, std::chrono::duration<Rep, Period> const& dur)
    {
        { t.try_lock_for(dur) } -> std::convertible_to<bool>;
    }
;

… and for the second constructor:

template<typename T, typename Clock, typename Duration>
concept ??? =
    requires(T& t, std::chrono::time_point<Clock, Duration> const& tp)
    {
        { t.try_lock_until(tp) } -> std::convertible_to<bool>;
    }
;

You can see how writing the concepts this way, we no longer need to worry about shenanigans to cover all timed lockable cases.

So we could just use the concepts outlined above, suitably named, but hark back to the design discussion about whether try_lock_for/until should do deadlock avoidance. I think it should, and if so, then we should also require the lockables to have try_lock support.

And not only that, we should allow for delegating try_lock_for() to try_lock_until(), and vice versa. This is not a particularly onerous requirement; if a lockable supports try_lock_for(), there is no sensible reason it should not also be able to support try_lock_until(), and vice versa.

However, codifying that requirement in the general case is challenging, if not practically impossible. It’s also not really necessary, in practice, because we will never want to delegate a call to try_lock_for() to try_lock_until() using, say, the GPS clock. We will only ever use a steady clock for such delegation.

Which means, at least for this use case, this should suffice:

template<typename T>
concept timed_lockable =
    lockable
    and requires(
        T& t,
        std::chrono::steady_clock::duration const& dur,
        std::chrono::steady_clock::time_point const& tp
    )
    {
        { t.try_lock_for(dur) } -> std::convertible_to<bool>;
        { t.try_lock_until(tp) } -> std::convertible_to<bool>;
    }
;

But I’m not sure that this concept, nor any of the other ones you already have, really make sense as general-use concepts. They seem more like implementation details for this specific class (and the specific functions). It is the standard that defines the lockable, basic lockable, etc. concepts, so it should be up to the standard to actually provide the concept concepts for them. It’s too bad it does not. The concepts you make to cover for that deficiency should really be details, not part of the public API.

That’s about all I can think for the class itself, except to mention that the swap function should probably be implemented as a hidden friend.

So I guess that about covers it. Hope it helped!