Merge pull request #4139 from pleroy/4136

Avoid a race in the construction of PushPullExecutor
mockingbirdnest · Nov 24, 2024 · 8fabf2d · 8fabf2d
2 parents e384167 + a929461
commit 8fabf2d
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 3 deletions.
diff --git a/base/push_pull_callback.hpp b/base/push_pull_callback.hpp
@@ -80,9 +80,11 @@ class PushPullExecutor {
 
  private:
   PushPullCallback<Result, Arguments...> callback_;
-  std::thread thread_;
   mutable absl::Mutex lock_;
   std::optional<T> result_ GUARDED_BY(lock_);
+
+  // This must come last as it references the other member variables, see #4136.
+  std::thread thread_;
 };
 
 }  // namespace internal

diff --git a/base/push_pull_callback_body.hpp b/base/push_pull_callback_body.hpp
@@ -5,6 +5,8 @@
 #include <memory>
 #include <utility>
 
+#include "glog/logging.h"
+
 namespace principia {
 namespace base {
 namespace _push_pull_callback {
@@ -103,20 +105,19 @@ PushPullExecutor<T, Result, Arguments...>::~PushPullExecutor() {
 template<typename T, typename Result, typename... Arguments>
 PushPullCallback<Result, Arguments...>&
 PushPullExecutor<T, Result, Arguments...>::callback() {
-  absl::ReaderMutexLock l(&lock_);
   return callback_;
 }
 
 template<typename T, typename Result, typename... Arguments>
 PushPullCallback<Result, Arguments...> const&
 PushPullExecutor<T, Result, Arguments...>::callback() const {
-  absl::ReaderMutexLock l(&lock_);
   return callback_;
 }
 
 template<typename T, typename Result, typename... Arguments>
 T PushPullExecutor<T, Result, Arguments...>::get() {
   absl::MutexLock l(&lock_);
+  CHECK(result_.has_value());
   return std::move(result_.value());
 }
 

diff --git a/base/push_pull_callback_test.cpp b/base/push_pull_callback_test.cpp
@@ -38,5 +38,28 @@ TEST(PushPullCallback, Test) {
   EXPECT_EQ(8, executor.get());
 }
 
+TEST(PushPullCallback, 4136) {
+  auto task =
+      [](std::function<double(int const x)> const& f)
+      -> std::optional<double> {
+    return std::nullopt;
+  };
+
+  for (std::int64_t attempt = 0; attempt < 100'000; ++attempt) {
+    auto* const executor =
+        new PushPullExecutor<std::optional<double>, double, int>(task);
+    for (;;) {
+      int x;
+      bool const more = executor->callback().Pull(x);
+      if (!more) {
+        auto const result = executor->get();
+        delete executor;
+        break;
+      }
+      executor->callback().Push(1.0);
+    }
+  }
+}
+
 }  // namespace base
 }  // namespace principia