thevar1able/ClickHouse
1
0
Fork 0
mirror of https://github.com/ClickHouse/ClickHouse.git synced 2024-09-19 16:20:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Correct lock order

Browse Source
This commit is contained in:
Antonio Andelic 2024-09-06 15:41:04 +02:00
parent f44eaa808d
commit 03c7f3817b
2 changed files with 13 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
src/Coordination/KeeperStorage.cpp
View File

@ -521,9 +521,11 @@ template <class... Ts>
Overloaded(Ts...) -> Overloaded<Ts...>;
template<typename Container>
std::shared_ptr<typename Container::Node> KeeperStorage<Container>::UncommittedState::tryGetNodeFromStorage(StringRef path) const
std::shared_ptr<typename Container::Node> KeeperStorage<Container>::UncommittedState::tryGetNodeFromStorage(StringRef path, bool should_lock_storage) const
{
std::shared_lock lock(storage.storage_mutex);
std::shared_lock lock(storage.storage_mutex, std::defer_lock);
if (should_lock_storage)
lock.lock();
if (auto node_it = storage.container.find(path); node_it != storage.container.end())
{
const auto & committed_node = node_it->value;
@ -841,12 +843,12 @@ void KeeperStorage<Container>::UncommittedState::rollback(std::list<Delta> rollb
}
template<typename Container>
std::shared_ptr<typename Container::Node> KeeperStorage<Container>::UncommittedState::getNode(StringRef path) const
std::shared_ptr<typename Container::Node> KeeperStorage<Container>::UncommittedState::getNode(StringRef path, bool should_lock_storage) const
{
if (auto node_it = nodes.find(path.toView()); node_it != nodes.end())
return node_it->second.node;
std::shared_ptr<KeeperStorage::Node> node = tryGetNodeFromStorage(path);
std::shared_ptr<KeeperStorage::Node> node = tryGetNodeFromStorage(path, should_lock_storage);
if (node)
{
@ -2648,7 +2650,7 @@ void KeeperStorage<Container>::preprocessRequest(
std::unordered_map<std::string_view, UpdateNodeStatDelta> parent_updates;
for (const auto & ephemeral_path : session_ephemerals->second)
{
auto node = uncommitted_state.getNode(ephemeral_path);
auto node = uncommitted_state.getNode(ephemeral_path, /*should_lock_storage=*/false);
/// maybe the node is deleted or recreated with different session_id in the uncommitted state
if (!node || node->stats.ephemeralOwner() != session_id)
@ -2659,7 +2661,7 @@ void KeeperStorage<Container>::preprocessRequest(
auto parent_update_it = parent_updates.find(parent_node_path);
if (parent_update_it == parent_updates.end())
{
auto parent_node = uncommitted_state.getNode(StringRef{parent_node_path});
auto parent_node = uncommitted_state.getNode(StringRef{parent_node_path}, /*should_lock_storage=*/false);
std::tie(parent_update_it, std::ignore) = parent_updates.emplace(parent_node_path, *parent_node);
}
@ -2684,6 +2686,9 @@ void KeeperStorage<Container>::preprocessRequest(
}
}
};
/// storage lock should always be taken before ephemeral lock
std::shared_lock storage_lock(storage_mutex);
std::lock_guard ephemeral_lock(ephemeral_mutex);
process_ephemerals_for_session(committed_ephemerals);
process_ephemerals_for_session(uncommitted_state.ephemerals);

4
src/Coordination/KeeperStorage.h
View File

@ -492,7 +492,7 @@ public:
void rollback(int64_t rollback_zxid);
void rollback(std::list<Delta> rollback_deltas);
std::shared_ptr<Node> getNode(StringRef path) const;
std::shared_ptr<Node> getNode(StringRef path, bool should_lock_storage = true) const;
Coordination::ACLs getACLs(StringRef path) const;
void applyDeltas(const std::list<Delta> & new_deltas);
@ -503,7 +503,7 @@ public:
void forEachAuthInSession(int64_t session_id, std::function<void(const AuthID &)> func) const;
std::shared_ptr<Node> tryGetNodeFromStorage(StringRef path) const;
std::shared_ptr<Node> tryGetNodeFromStorage(StringRef path, bool should_lock_storage = true) const;
std::unordered_map<int64_t, std::list<std::pair<int64_t, std::shared_ptr<AuthID>>>> session_and_auth;
std::unordered_set<int64_t> closed_sessions;