[arrow] branch master updated: ARROW-11376: [C++] ThreadedTaskGroup failure with Thread Sanitizer enabled

[ap...@apache.org]

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

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


The following commit(s) were added to refs/heads/master by this push:
     new 384023e  ARROW-11376: [C++] ThreadedTaskGroup failure with Thread Sanitizer enabled
384023e is described below

commit 384023e44fca41d957924b8a320d4df39e5753a8
Author: Weston Pace <we...@gmail.com>
AuthorDate: Tue Jan 26 18:05:31 2021 +0100

    ARROW-11376: [C++] ThreadedTaskGroup failure with Thread Sanitizer enabled
    
    In the test we were checking if the outer task set some varable before waiting on the task group.  This was a potential data race and potential false positive.  I fixed the initial condition to false to avoid false positives and then moved the check after the finish to avoid the data race.  Also, while investigating, I noticed that the ok() method could be const but wasn't (at one point I thought this might be related to the issue) so I changed that.
    
    Closes #9319 from westonpace/bugfix/arrow-11371
    
    Authored-by: Weston Pace <we...@gmail.com>
    Signed-off-by: Antoine Pitrou <an...@python.org>
---
 cpp/src/arrow/util/task_group.cc      | 4 ++--
 cpp/src/arrow/util/task_group.h       | 2 +-
 cpp/src/arrow/util/task_group_test.cc | 5 ++---
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/cpp/src/arrow/util/task_group.cc b/cpp/src/arrow/util/task_group.cc
index 76f39c9..8765602 100644
--- a/cpp/src/arrow/util/task_group.cc
+++ b/cpp/src/arrow/util/task_group.cc
@@ -45,7 +45,7 @@ class SerialTaskGroup : public TaskGroup {
 
   Status current_status() override { return status_; }
 
-  bool ok() override { return status_.ok(); }
+  bool ok() const override { return status_.ok(); }
 
   Status Finish() override {
     if (!finished_) {
@@ -102,7 +102,7 @@ class ThreadedTaskGroup : public TaskGroup {
     return status_;
   }
 
-  bool ok() override { return ok_.load(); }
+  bool ok() const override { return ok_.load(); }
 
   Status Finish() override {
     std::unique_lock<std::mutex> lock(mutex_);
diff --git a/cpp/src/arrow/util/task_group.h b/cpp/src/arrow/util/task_group.h
index 1ab9b81..db3265d 100644
--- a/cpp/src/arrow/util/task_group.h
+++ b/cpp/src/arrow/util/task_group.h
@@ -67,7 +67,7 @@ class ARROW_EXPORT TaskGroup : public std::enable_shared_from_this<TaskGroup> {
   virtual Status current_status() = 0;
 
   /// Whether some tasks have already failed.  Non-blocking, useful for stopping early.
-  virtual bool ok() = 0;
+  virtual bool ok() const = 0;
 
   /// How many tasks can typically be executed in parallel.
   /// This is only a hint, useful for testing or debugging.
diff --git a/cpp/src/arrow/util/task_group_test.cc b/cpp/src/arrow/util/task_group_test.cc
index 815e22e..1e47a34 100644
--- a/cpp/src/arrow/util/task_group_test.cc
+++ b/cpp/src/arrow/util/task_group_test.cc
@@ -77,7 +77,7 @@ void TestTaskGroupErrors(std::shared_ptr<TaskGroup> task_group) {
 
   std::atomic<int> count(0);
 
-  auto task_group_was_ok = true;
+  auto task_group_was_ok = false;
   task_group->Append([&]() -> Status {
     for (int i = 0; i < NSUCCESSES; ++i) {
       task_group->Append([&]() {
@@ -97,10 +97,9 @@ void TestTaskGroupErrors(std::shared_ptr<TaskGroup> task_group) {
     return Status::OK();
   });
 
-  ASSERT_TRUE(task_group_was_ok);
-
   // Task error is propagated
   ASSERT_RAISES(Invalid, task_group->Finish());
+  ASSERT_TRUE(task_group_was_ok);
   ASSERT_FALSE(task_group->ok());
   if (task_group->parallelism() == 1) {
     // Serial: exactly two successes and an error