[impala] branch master updated: IMPALA-12411: Fix data race in expr-test teardown

[db...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dbecker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new 900f9f057 IMPALA-12411: Fix data race in expr-test teardown
900f9f057 is described below

commit 900f9f057c6814cda57f71049fc4de9348ff7163
Author: Michael Smith <mi...@cloudera.com>
AuthorDate: Mon Sep 11 16:30:54 2023 -0700

    IMPALA-12411: Fix data race in expr-test teardown
    
    TSAN ThreadSanitizer detected a data race during expr-test teardown when
    a query finishes after JvmMetricCache's destructor is called. expr-test
    can finish with a query still running because ImpaladQueryExecutor
    closes the previous query when you start the next query and relies on
    the destructor to close the last query.
    
    expr-test creates ImpaladQueryExecutor as a global as part of setting up
    an in-memory cluster (with Statestore and InProcessImpalaServer). So the
    last query isn't guaranteed to be closed until global destruction, which
    leads to a race with other global destruction (in this case with
    JvmMetricCache while updating QueryState on query completion).
    
    ImpaladQueryExecutor is only used in expr-test.
    
    Change-Id: I8e289b292cb11154f1245e2b987cde0995b6243c
    Reviewed-on: http://gerrit.cloudera.org:8080/20474
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/exprs/expr-test.cc | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 5edd34e65..e017b1134 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -97,7 +97,7 @@ namespace impala {
 // Use this timezone in tests where DST changes may cause problems.
 const char* TEST_TZ_WITHOUT_DST = "America/Anguilla";
 
-ImpaladQueryExecutor* executor_;
+scoped_ptr<ImpaladQueryExecutor> executor_;
 scoped_ptr<MetricGroup> statestore_metrics(new MetricGroup("statestore_metrics"));
 Statestore* statestore;
 
@@ -232,10 +232,17 @@ class ExprTest : public testing::TestWithParam<std::tuple<bool, bool>> {
         FLAGS_hostname, statestore->port(), &impala_server));
     IGNORE_LEAKING_OBJECT(impala_server);
 
-    executor_ = new ImpaladQueryExecutor(FLAGS_hostname, impala_server->GetBeeswaxPort());
+    executor_.reset(
+        new ImpaladQueryExecutor(FLAGS_hostname, impala_server->GetBeeswaxPort()));
     ABORT_IF_ERROR(executor_->Setup());
   }
 
+  static void TearDownTestCase() {
+    // Teardown before global destructors to avoid a race where JvmMetricCache is
+    // destroyed before the last query is closed.
+    executor_.reset();
+  }
+
  protected:
   // Pool for objects to be destroyed during test teardown.
   ObjectPool pool_;
@@ -7003,7 +7010,7 @@ TEST_P(ExprTest, TimestampFunctions) {
   // Test Unix epoch conversions again but now converting into local timestamp values.
   {
     ScopedTimeZoneOverride time_zone("PST8PDT");
-    ScopedExecOption use_local(executor_,
+    ScopedExecOption use_local(executor_.get(),
         "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
     // Determine what the local time would have been when it was 1970-01-01 GMT
     ptime local_time_at_epoch = c_local_adjustor<ptime>::utc_to_local(from_time_t(0));
@@ -7242,7 +7249,7 @@ TEST_P(ExprTest, TimestampFunctions) {
   // Tests from Hive. When casting from timestamp to numeric, timestamps are considered
   // to be local values.
   {
-    ScopedExecOption use_local(executor_,
+    ScopedExecOption use_local(executor_.get(),
         "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
     ScopedTimeZoneOverride time_zone("PST8PDT");
     TestValue("cast(cast('2011-01-01 01:01:01' as timestamp) as boolean)", TYPE_BOOLEAN,
@@ -7316,7 +7323,7 @@ TEST_P(ExprTest, TimestampFunctions) {
 
     // Check again with the flag enabled.
     {
-      ScopedExecOption use_local(executor_,
+      ScopedExecOption use_local(executor_.get(),
           "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
       tm before = to_tm(posix_time::microsec_clock::local_time());
       unix_start_time = mktime(&before);
@@ -7331,7 +7338,7 @@ TEST_P(ExprTest, TimestampFunctions) {
   // Test that now() and current_timestamp() are reasonable.
   {
     ScopedTimeZoneOverride time_zone(TEST_TZ_WITHOUT_DST);
-    ScopedExecOption use_local(executor_,
+    ScopedExecOption use_local(executor_.get(),
         "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
     const Timezone& local_tz = time_zone.GetTimezone();
 
@@ -7358,7 +7365,7 @@ TEST_P(ExprTest, TimestampFunctions) {
   // Test cast(unix_timestamp() as timestamp).
   {
     ScopedTimeZoneOverride time_zone(TEST_TZ_WITHOUT_DST);
-    ScopedExecOption use_local(executor_,
+    ScopedExecOption use_local(executor_.get(),
         "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
     const Timezone& local_tz = time_zone.GetTimezone();
 
@@ -8511,7 +8518,7 @@ TEST_P(ExprTest, DateFunctions) {
   // Test that current_date() is reasonable.
   {
     ScopedTimeZoneOverride time_zone(TEST_TZ_WITHOUT_DST);
-    ScopedExecOption use_local(executor_,
+    ScopedExecOption use_local(executor_.get(),
         "USE_LOCAL_TZ_FOR_UNIX_TIMESTAMP_CONVERSIONS=1");
     const Timezone& local_tz = time_zone.GetTimezone();