[impala] 03/03: IMPALA-8360: Fix race conditions in thread-pool-test

[jo...@apache.org]

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

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

commit 80b82b5b2c60ff934eabe42dd145c9f84ca3a60b
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Thu Mar 28 15:38:23 2019 -0700

    IMPALA-8360: Fix race conditions in thread-pool-test
    
    There are race conditions in thread-pool-test between the caller
    thread and the worker thread. Specifically, in some cases, the
    worker thread seems to be slow in freeing resources. This can
    lead to asserts failing because the work item has not been
    freed or because the submit of a task failed when it should
    not have.
    
    This fixes the race conditions:
     - It breaks up the existing SynchronousThreadPoolTest into two
       smaller tests so that the two can't interfere with each other.
     - It adds the ability to sleep and recheck that the work item
       has been freed.
    
    Testing:
     - Ran thread-pool-test in a loop for 20k iterations
    
    Change-Id: Id2d5f8b677e475d8e9d6b4512e990b20bfbefaf1
    Reviewed-on: http://gerrit.cloudera.org:8080/12916
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/util/thread-pool-test.cc | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/be/src/util/thread-pool-test.cc b/be/src/util/thread-pool-test.cc
index e02c5a2..3633ae9 100644
--- a/be/src/util/thread-pool-test.cc
+++ b/be/src/util/thread-pool-test.cc
@@ -95,7 +95,7 @@ private:
   bool* destructor_called_ptr_;
 };
 
-TEST(ThreadPoolTest, SynchronousThreadPoolTest) {
+TEST(ThreadPoolTest, SynchronousThreadPoolNoSleep) {
   // Create a synchronous pool with one thread and a queue size of one.
   SynchronousThreadPool pool("sync-thread-pool", "worker", 1, 1);
   ASSERT_OK(pool.Init());
@@ -109,7 +109,22 @@ TEST(ThreadPoolTest, SynchronousThreadPoolTest) {
   // shared_ptr to the work item. The caller is the only holder, so when it calls
   // reset, the destructor must be called.
   no_sleep.reset();
+  // The work item should be destroyed even if we are not shutting down the pool.
+  // IMPALA-8371: There is a race condition with the worker thread, as the worker thread
+  // may not have released its shared_ptr to the work item. Wait for a limited period of
+  // time for the work thread to release the shared_ptr.
+  for (int i = 0; i < 10; i++) {
+    if (*no_sleep_destroyed) break;
+    SleepForMs(5);
+  }
   ASSERT_TRUE(*no_sleep_destroyed);
+  pool.DrainAndShutdown();
+}
+
+TEST(ThreadPoolTest, SynchronousThreadPoolTimeouts) {
+  // Create a synchronous pool with one thread and a queue size of one.
+  SynchronousThreadPool pool("sync-thread-pool", "worker", 1, 1);
+  ASSERT_OK(pool.Init());
 
   // Timeout case #1: Submit one task that takes 100 milliseconds. Offer it with a timeout
   // of 1 millisecond so that the caller immediately times out.