From: Kefu Chai <kchai@redhat.com>
Date: Fri, 16 Oct 2020 06:11:52 +0000 (+0800)
Subject: crimson/common: do not take from a future twice
X-Git-Tag: v17.0.0~866^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=5d80c545a95a343c5cf8b07ad40af4a955136e36;p=ceph.git

crimson/common: do not take from a future twice

before this change, in our specialization of seastar::do_until(),
we access `f` after calling `f.get()`, this is not correct. as `f.get()`
actually moves `f._state` away and detaches the associated promise if any.
so we cannot call `f._then()` anymore after calling `f.get()`. as
`f._then()` schedules `f` by detaching the future from promise and
attaching the scheduled task to the promise. but `future_base::detach_promise()`
does not check `_promise` before accessing it, hence the segfault.

after this change, the order of the checks is rearranged so that
`f.get()` is called at the end. and also use `f.get0()` to be more
explicit, as we are accessing the only element of the returned
value.

Signed-off-by: Kefu Chai <kchai@redhat.com>
---

diff --git a/src/crimson/common/errorator.h b/src/crimson/common/errorator.h
index ea0ffea4d4915..a52a711f9d1bb 100644
--- a/src/crimson/common/errorator.h
+++ b/src/crimson/common/errorator.h
@@ -54,8 +54,10 @@ inline auto do_until(AsyncAction action) {
 
   while (true) {
     auto f = futurator::invoke(action);
-    if (!seastar::need_preempt() && f.available() && f.get()) {
-      return futurator::type::errorator_type::template make_ready_future<>();
+    if (f.failed()) {
+      return futurator::type::errorator_type::template make_exception_future2<>(
+        f.get_exception()
+      );
     }
     if (!f.available() || seastar::need_preempt()) {
       return std::move(f)._then(
@@ -66,11 +68,8 @@ inline auto do_until(AsyncAction action) {
           return ::crimson::do_until(
 	    std::move(action));
 	});
-    }
-    if (f.failed()) {
-      return futurator::type::errorator_type::template make_exception_future2<>(
-	f.get_exception()
-      );
+    } else if (bool stop_cond = f.get0()) {
+      return futurator::type::errorator_type::template make_ready_future<>();
     }
   }
 }
diff --git a/src/test/crimson/CMakeLists.txt b/src/test/crimson/CMakeLists.txt
index 86eb6fb7785eb..5d2caa1191098 100644
--- a/src/test/crimson/CMakeLists.txt
+++ b/src/test/crimson/CMakeLists.txt
@@ -71,3 +71,10 @@ add_library(crimson-gtest STATIC
   gtest_seastar.cc)
 target_link_libraries(crimson-gtest crimson-common GTest::GTest)
 add_library(crimson::gtest ALIAS crimson-gtest)
+
+add_executable(unittest-seastar-errorator
+  test_errorator.cc)
+target_link_libraries(
+  unittest-seastar-errorator
+  crimson::gtest)
+
diff --git a/src/test/crimson/test_errorator.cc b/src/test/crimson/test_errorator.cc
new file mode 100644
index 0000000000000..08f053725be63
--- /dev/null
+++ b/src/test/crimson/test_errorator.cc
@@ -0,0 +1,32 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
+// vim: ts=8 sw=2 smarttab
+
+#include "test/crimson/gtest_seastar.h"
+
+#include "crimson/common/errorator.h"
+#include "crimson/common/log.h"
+
+struct do_until_test_t : public seastar_test_suite_t {
+  using ertr = crimson::errorator<crimson::ct_error::invarg>;
+  template <class... ValuesT>
+  using future = ertr::future<ValuesT...>;
+  future<> test() {
+    return crimson::do_until([this]() -> future<bool> {
+      if (i < 5) {
+        ++i;
+        return ertr::make_ready_future<bool>(false);
+      } else {
+        return ertr::make_ready_future<bool>(true);
+      }
+    });
+  }
+
+  int i = 0;
+};
+
+TEST_F(do_until_test_t, basic)
+{
+  run_async([this] {
+    test().unsafe_get0();
+  });
+}