commit 9f51673a40d61cf087dd72c6e50719ed522ac851
author Qifan Chen <qchen@cloudera.com> Wed Sep 02 23:33:50 2020 -0400
committer Impala Public Jenkins <impala-public-jenkins@cloudera.com> Wed Sep 09 04:23:17 2020 +0000
tree 971869bf71ea47889fd25620760740e987fc99ae
parent 6aaea3216ca2fabe352b9f11a7d140e9d9c183c9

IMPALA-10129 Data race in MemTracker::GetTopNQueriesAndUpdatePoolStats

This work addresses a data race condition in admission controller by
providing the initializing values for two data members (
is_query_mem_tracker_ and query_id_) in a constructor for the MemTracker
class. Without doing so, the two data members are set, without lock
protection, after the object is constructed, which allows other threads
to modify either of them at the same time.

Testing:
1. Ran the python admission controller test successfully with a tsan
   build. Data race was not observed with the enhancement. Data race
   was observed without the enhancement.
2. Ran the core test.

Change-Id: I9c4ffe8064d3e099a525cc48c218ef73112fb67b
Reviewed-on: http://gerrit.cloudera.org:8080/16408
Reviewed-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>

be/src/runtime/mem-tracker.cc

diff --git a/be/src/runtime/mem-tracker.cc b/be/src/runtime/mem-tracker.cc
index 96fd3f9..1521d41 100644
--- a/be/src/runtime/mem-tracker.cc
+++ b/be/src/runtime/mem-tracker.cc
@@ -57,9 +57,11 @@
   return static_cast<int64_t>(limit * frac);
 }
 
-MemTracker::MemTracker(
-    int64_t byte_limit, const string& label, MemTracker* parent, bool log_usage_if_zero)
-  : limit_(byte_limit),
+MemTracker::MemTracker(int64_t byte_limit, const string& label, MemTracker* parent,
+    bool log_usage_if_zero, bool is_query_mem_tracker, const TUniqueId& query_id)
+  : is_query_mem_tracker_(is_query_mem_tracker),
+    query_id_(query_id),
+    limit_(byte_limit),
     soft_limit_(CalcSoftLimit(byte_limit)),
     label_(label),
     parent_(parent),
@@ -231,9 +233,7 @@
       ExecEnv::GetInstance()->pool_mem_trackers()->GetRequestPoolMemTracker(
           pool_name, true);
   MemTracker* tracker = obj_pool->Add(new MemTracker(
-      mem_limit, Substitute("Query($0)", PrintId(id)), pool_tracker));
-  tracker->is_query_mem_tracker_ = true;
-  tracker->query_id_ = id;
+      mem_limit, Substitute("Query($0)", PrintId(id)), pool_tracker, true, true, id));
   return tracker;
 }
 

be/src/runtime/mem-tracker.h

diff --git a/be/src/runtime/mem-tracker.h b/be/src/runtime/mem-tracker.h
index 1e13465..ea2fecb 100644
--- a/be/src/runtime/mem-tracker.h
+++ b/be/src/runtime/mem-tracker.h
@@ -93,7 +93,8 @@
   /// included
   /// in LogUsage() output if consumption is 0.
   MemTracker(int64_t byte_limit = -1, const std::string& label = std::string(),
-      MemTracker* parent = nullptr, bool log_usage_if_zero = true);
+      MemTracker* parent = nullptr, bool log_usage_if_zero = true,
+      bool is_query_mem_tracker = false, const TUniqueId& query_id = TUniqueId());
 
   /// C'tor for tracker for which consumption counter is created as part of a profile.
   /// The counter is created with name COUNTER_NAME.