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 ea0ffea4d49151e82f92d963594b869c3a80f770..a52a711f9d1bbdff4d8792c7fbe3da77e696d85a 100644 (file)
--- 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 86eb6fb7785eba5f416f4e5747ce3c9318d03b23..5d2caa1191098a42d34b0ffcac1c58cc549c6a93 100644 (file)
--- 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 (file)
index 0000000..08f0537
--- /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();
+  });
+}