Fix data race when updating the slots_info string (apache#1615)

Co-authored-by: Twice <twice@apache.org>
p1u3o · Aug 1, 2023 · ceaa80e · ceaa80e
1 parent 8f44f7b
commit ceaa80e
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 19 deletions.
diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc
@@ -472,7 +472,7 @@ Status Cluster::GetClusterNodes(std::string *nodes_str) {
 }
 
 std::string Cluster::genNodesDescription() {
-  updateSlotsInfo();
+  auto slots_infos = getClusterNodeSlots();
 
   auto now = util::GetTimeStampMS();
   std::string nodes_desc;
@@ -495,22 +495,22 @@ std::string Cluster::genNodesDescription() {
     // Ping sent, pong received, config epoch, link status
     node_str.append(fmt::format("{} {} {} connected", now - 1, now, version_));
 
-    if (n->role == kClusterMaster && n->slots_info.size() > 0) {
-      node_str.append(" " + n->slots_info);
+    if (n->role == kClusterMaster) {
+      auto iter = slots_infos.find(n->id);
+      if (iter != slots_infos.end() && iter->second.size() > 0) {
+        node_str.append(" " + iter->second);
+      }
     }
 
     nodes_desc.append(node_str + "\n");
   }
   return nodes_desc;
 }
 
-void Cluster::updateSlotsInfo() {
+std::map<std::string, std::string> Cluster::getClusterNodeSlots() const {
   int start = -1;
-  // reset the previous slots info
-  for (const auto &item : nodes_) {
-    const std::shared_ptr<ClusterNode> &n = item.second;
-    n->slots_info.clear();
-  }
+  // node id => slots info string
+  std::map<std::string, std::string> slots_infos;
 
   std::shared_ptr<ClusterNode> n = nullptr;
   for (int i = 0; i <= kClusterSlots; i++) {
@@ -524,24 +524,24 @@ void Cluster::updateSlotsInfo() {
     // Generate slots info when occur different node with start or end of slot
     if (i == kClusterSlots || n != slots_nodes_[i]) {
       if (start == i - 1) {
-        n->slots_info += fmt::format("{} ", start);
+        slots_infos[n->id] += fmt::format("{} ", start);
       } else {
-        n->slots_info += fmt::format("{}-{} ", start, i - 1);
+        slots_infos[n->id] += fmt::format("{}-{} ", start, i - 1);
       }
       if (i == kClusterSlots) break;
       n = slots_nodes_[i];
       start = i;
     }
   }
 
-  for (const auto &item : nodes_) {
-    const std::shared_ptr<ClusterNode> n = item.second;
-    if (n->slots_info.size() > 0) n->slots_info.pop_back();  // Remove last space
+  for (auto &[_, info] : slots_infos) {
+    if (info.size() > 0) info.pop_back();  // Remove last space
   }
+  return slots_infos;
 }
 
 std::string Cluster::genNodesInfo() {
-  updateSlotsInfo();
+  auto slots_infos = getClusterNodeSlots();
 
   std::string nodes_info;
   for (const auto &item : nodes_) {
@@ -561,8 +561,11 @@ std::string Cluster::genNodesInfo() {
     }
 
     // Slots
-    if (n->role == kClusterMaster && n->slots_info.size() > 0) {
-      node_str.append(" " + n->slots_info);
+    if (n->role == kClusterMaster) {
+      auto iter = slots_infos.find(n->id);
+      if (iter != slots_infos.end() && iter->second.size() > 0) {
+        node_str.append(" " + iter->second);
+      }
     }
     nodes_info.append(node_str + "\n");
   }

diff --git a/src/cluster/cluster.h b/src/cluster/cluster.h
@@ -45,7 +45,6 @@ class ClusterNode {
   int port;
   int role;
   std::string master_id;
-  std::string slots_info;
   std::bitset<kClusterSlots> slots;
   std::vector<std::string> replicas;
   int importing_slot = -1;
@@ -96,7 +95,7 @@ class Cluster {
  private:
   std::string genNodesDescription();
   std::string genNodesInfo();
-  void updateSlotsInfo();
+  std::map<std::string, std::string> getClusterNodeSlots() const;
   SlotInfo genSlotNodeInfo(int start, int end, const std::shared_ptr<ClusterNode> &n);
   static Status parseClusterNodes(const std::string &nodes_str, ClusterNodes *nodes,
                                   std::unordered_map<int, std::string> *slots_nodes);