Taking multiple locks within nested loops in cases where order cannot be anticipated

Viewed 25

I have a graph of cross-referenced objects holding data that needs to be synchronized, and there are several threads applying "exploratory operations" over the graph.

This takes me to a situation where threads need to take the lock of a node to explore connected nodes and subsequently take theirs locks.

Is there a recommended way of doing this? without risking invalidating loops and applying deadlock avoidance?

Here is a minimal example of this with my initial approach, exploratory_operation shows the dilemma:

#include <mutex>
#include <shared_mutex>
#include <memory>
#include <vector>
#include <iostream>

template<class T>
class Node {

    public:

        Node(const T& data) : data_(data)
        {}

        void addNode(const std::shared_ptr<Node<T>>& node)
        {
            // Connects to a new node safely holding a unique lock
            std::unique_lock<std::shared_mutex> lock(mutex);
            unsafe_addNode(node);
        }

        void unsafe_addNode(const std::shared_ptr<Node<T>>& node)
        {
            connections_.push_back(node);
        }

        std::vector<std::shared_ptr<Node>> getNodes()
        {
            // Returns a copy of connections safely holding a shared lock
            std::shared_lock<std::shared_mutex> lock(mutex);
            return unsafe_getNodes();
        }

        std::vector<std::shared_ptr<Node>>& unsafe_getNodes()
        {
            return connections_;
        }

        T getData()
        {
            // Makes a copy safely holding a shared lock
            std::shared_lock<std::shared_mutex> lock(mutex);
            return unsafe_getData();
        }

        T& unsafe_getData()
        {
            return data_;
        }

        mutable std::shared_mutex mutex;

    private:
        std::vector<std::shared_ptr<Node>> connections_;

        T data_;
};

void exploratory_operation(const std::shared_ptr<Node<int>>& node)
{
    /* 
        * Suppose that you cannot anticipate connections and that may exist
        * several other threads doing operations over nodes in aleatory order
        */

    // Taking node lock to read connections and execute unsafe functions
    std::shared_lock<std::shared_mutex> lock(node->mutex);

    // We don't want to copy the entire set of connections using getNodes()
    for(const auto& other_node : node->unsafe_getNodes())
    {
        std::shared_lock<std::shared_mutex> other_lock(other_node->mutex, std::defer_lock);

        // Unlocking first as otherwise std::lock is UNDEFINED BEHAVIOR (lock is already taken)
        lock.unlock();
        // The for-loop may become invalidated in between (some another thread may add nodes and trigger vector's reallocation)
        std::lock(lock, other_lock); // Attempt locking with deadlock-avoidance

        for(const auto& other_other_node : other_node->unsafe_getNodes())
        {
            // These problems escalate on subsequent nested loops
            std::shared_lock<std::shared_mutex> other_other_lock(other_other_node->mutex, std::defer_lock);

            lock.unlock(); // to not get UB
            other_lock.unlock();

            std::lock(lock, other_lock, other_other_lock); // deadlock-avoidance

            // Working with data...
            std::cout << "Datas: " << node->unsafe_getData() << " " << other_node->unsafe_getData() << " " << other_other_node->unsafe_getData() << std::endl;
        }
    }
}

int main() {
    
    // Some example initialization
    std::shared_ptr<Node<int>> A = std::make_shared<Node<int>>(1);
    std::shared_ptr<Node<int>> B = std::make_shared<Node<int>>(2);
    std::shared_ptr<Node<int>> C = std::make_shared<Node<int>>(3);

    A->addNode(B);
    A->addNode(C);
    B->addNode(A);
    C->addNode(B);

    exploratory_operation(A);

};

Releasing locks and calling again std::lock is the best approach?

0 Answers
Related