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?