2

My code is like the following. A Element holds a pointer to a mutex, and there is a vector that holds all Elements.

If I know the count of elements at compile time, I can construct a scoped_lock to lock all these mutexes. However, I don't know the size of the vector now. So how can I lock all the mutexes and generate a lock guard safely?

#include <vector>
#include <mutex>

struct Element {
    Element() {
        mut = new std::mutex();
    }
    Element(Element && other) : mut(other.mut) {
        other.mut = nullptr;
    }
    ~Element() { delete mut; }
    std::mutex * mut;
};

int main() {
    std::vector<Element> v;
    v.push_back(Element());
    v.push_back(Element());
    v.push_back(Element());

    std::scoped_lock(*v[0].mut, *v[1].mut, *v[2].mut);
}
Aamir
  • 1,974
  • 1
  • 14
  • 18
calvin
  • 2,125
  • 2
  • 21
  • 38
  • 4
    This seems more like a design issue in your code. Is it possible to fix that? Why would you need to store pointers to mutexes? This makes idiomatic usage of RAII difficult. Related: https://stackoverflow.com/questions/69366623/stdscoped-lock-vector-of-mutexes – Cody Gray - on strike May 26 '23 at 11:58
  • 1
    That sounds wrong. Why would anyone need to lock all the mutexes at the same time? Why does `Element` expose its mutex as public instead of hiding it as implementation detail and exposing functions that will lock mutex internally when needed? – Yksisarvinen May 26 '23 at 11:59
  • By the way, the move constructor seems wrong. You should set the "copied from" `mut` to `nullptr` (use `std::exchange` for example) otherwise you'll have both the "copied from" and the "copied to" objects referring to the same mutex. – Fareanor May 26 '23 at 12:05
  • You could still iterate yourself over the vector and call `std::lock` for each mutex (but doing them separately, I guess you'll lose the deadlock avoidance feature) – Fareanor May 26 '23 at 12:22
  • @calvin You should also add a destructor that will delete the pointer :p – Fareanor May 26 '23 at 12:33
  • If you have a well-defined order on a list of mutexes, then locking them in that order is the correct thing to do. Just write a RAII wrapper around that. – Passer By May 26 '23 at 12:37
  • Or you could have used `std::unique_ptr` instead (and move it in your move constructor). – Fareanor May 26 '23 at 12:37
  • @Fareanor The vector is immutable once it is constructed. So is it deadlock avoidance that I sort the mutex pointers in order, and use a loop to lock them in that order? – calvin May 26 '23 at 12:38
  • @calvin AFAIK, If the order of lock is guaranteed to be preserved, you don't need the deadlock avoidance algorithm. – Fareanor May 26 '23 at 12:40
  • You don't need to sort the mutex pointers, you just need to make sure that the locks are acquired in a predictable, consistent order (and released in reversed order). The order in which they are stored in the vector is already fine, without sorting. I suggest reading about the dining philosophers problem, and the [solution that uses a hierarchy](https://en.wikipedia.org/wiki/Dining_philosophers_problem#Resource_hierarchy_solution). – Fabio says Reinstate Monica May 26 '23 at 13:11
  • @FabiosaysReinstateMonica Only the order of acquisition matters (you don't need to release in reverse order). – Fareanor May 26 '23 at 13:18
  • @Fareanor You are right, sorry! It's funny, it's even written at the link that I've provided. Looks like the OP isn't the only one who should read it :-D – Fabio says Reinstate Monica May 26 '23 at 13:23
  • 1
    @FabiosaysReinstateMonica You also don't need to be sorry :p You're welcome ! – Fareanor May 26 '23 at 13:26

3 Answers3

1

Well, you may try this cursed code:

struct MultiScopedLock {
  template <std::input_iterator I, std::sentinel_for<I> S>
  requires std::is_same_v<decltype(*std::declval<I>()), std::mutex &>
  MultiScopedLock(I begin, S end) {
    if constexpr (
        requires {requires std::random_access_iterator<I>; }
        && std::is_same_v<std::remove_cv<I>, std::remove_cv<S>>
    ){
        guards.reserve(end - begin);
    }
    try {
      for (; begin != end; ++begin) {
        guards.emplace_back(*begin);
      }
    } catch (...) {
      clear_guards();
      throw;
    }
  }

  // Need to delete all of this to guarantee unlocking order.
  MultiScopedLock(const MultiScopedLock &) = delete;
  MultiScopedLock(MultiScopedLock &&) = delete;
  MultiScopedLock &operator=(const MultiScopedLock &) = delete;
  MultiScopedLock &operator=(MultiScopedLock &&) = delete;

  ~MultiScopedLock() { clear_guards(); }

private:
  std::vector<std::unique_lock<std::mutex>> guards;

  void clear_guards() {
    // Release locks in reverse order.
    while (!guards.empty()) {
      guards.pop_back();
    }
  }
};

P.S. Thanks to @davis-herring for std::unique_lock suggestion.

0

I don't really see any use case where we would need to store a bunch of mutexes and lock/unlock them all in a row, but anyway, you could write your own "lock_guard" that will lock/unlock them all by iterating over the std::vector.

Note: If you cannot preserve the ordering of the locks, this solution will be subject to deadlock issues.


Example

Considering the following Element class (I just rewrote it in a simpler way):

struct Element
{
    std::unique_ptr<std::mutex> mut;

    Element() = default;
    Element(Element && other) : mut{std::move(other.mut)}
    {}
};

You could write your own "lock guard" as:

struct elements_lock
{
    std::vector<Element> & v_ref;

    elements_lock(std::vector<Element> & v) : v_ref{v}
    {
        for(Element & e : v)
            e.mut->lock();
    }
    ~elements_lock()
    {
        for(Element & e : v_ref)
            e.mut->unlock();
    }
};

And use it the following way:

int main()
{
    std::vector<Element> v(3); // Create a vector of 3 Elements
    {
        elements_lock el_lock(v);  // Acquire the locks sequentially

        // [...]

    } // At el_lock destruction, the locks will be released sequentially

    return 0;
}
Fareanor
  • 5,900
  • 2
  • 11
  • 37
  • Doesn't this need the same `catch(...)` safeguard as the other answers? – Davis Herring May 26 '23 at 20:29
  • @DavisHerring Actually, you may need to add a try-catch closure for the lock part, but not for the unlock part: `std::mutex::lock()` may throw `std::system_error` when an error occurs, but `std::mutex::unlock()` throws nothing. Anyway, I was only illustrating the concept. The "security" part was not the topic of the question (and could hurt a bit the readability), but of course, in "real" or production code, it would be preferable to create the `elements_lock` inside a try-catch closure instead of a plain block as I did here. Thanks for pointing that out though ! – Fareanor May 28 '23 at 11:13
0

You could lock te mutexes in an order of your choosing and unlock them in reverse order. Note that this doesn't guarantee that the order of locking is suitable for avoiding a deadlock when std::scoped_lock is used with multiple of these mutexes.


class [[nodiscard]] ElementsLock
{
    std::vector<std::mutex*> m_orderedMutexes;
public:
    // defining these results in deleted copy semantics
    ElementsLock(ElementsLock&&) noexcept = default;
    ElementsLock& operator=(ElementsLock&&) noexcept = default;

    ElementsLock(std::vector<Element> const& v)
    {
        m_orderedMutexes.reserve(v.size());
        for (auto& e : v)
        {
            m_orderedMutexes.push_back(e.mut);
        }
        std::sort(m_orderedMutexes.begin(), m_orderedMutexes.end(),
            [](void const* a, void const* b) { return reinterpret_cast<uintptr_t>(a) < reinterpret_cast<uintptr_t>(b); });

        auto lockPos = m_orderedMutexes.begin();
        try
        {
            for (auto const end = m_orderedMutexes.end(); lockPos != end; ++lockPos)
            {
                (**lockPos).lock();
            }
        }
        catch (...)
        {
            // unlock locked mutexes
            for (auto const begin = m_orderedMutexes.begin(); lockPos != begin;)
            {
                --lockPos;
                try
                {
                    (**lockPos).unlock();
                }
                catch (...)
                {
                    // cannot properly unlock the locked mutexes
                    std::terminate();
                }
            }

            throw;
        }
    }

    ~ElementsLock() noexcept
    {
        for (auto begin = m_orderedMutexes.begin(), pos = m_orderedMutexes.end(); pos != begin;)
        {
            --pos;
            (**pos).unlock();
        }
    }
};


int main() {
    std::vector<Element> v;
    v.push_back(Element());
    v.push_back(Element());
    v.push_back(Element());

    ElementsLock lock(v);
}
fabian
  • 80,457
  • 12
  • 86
  • 114