crimson/common/errorator: fix skipped aborts

We should also invoke the errfunc (which aborts) when the
return type is no_touch_error_marker.
Added comments explaining:
* why it's forbidden to return void
* why std::is_same_v<return_t, no_touch_error_marker> is checked

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>

diff --git a/src/crimson/common/errorator.h b/src/crimson/common/errorator.h
index 5d30d6f14d4cf3443f652623002fe342f53677b9..619ecd7c6e563a9738bac2fb01a36034f219de05 100644 (file)
--- a/src/crimson/common/errorator.h
+++ b/src/crimson/common/errorator.h
@@ -327,6 +327,10 @@ struct stateful_error_t : error_t<stateful_error_t<ErrorT>> {
     }
   };
 
+  auto exception_ptr() {
+    return ep;
+  }
+
 private:
   std::exception_ptr ep;
 
@@ -366,11 +370,24 @@ public:
   void handle() {
     static_assert(std::is_invocable<ErrorVisitorT, ErrorT>::value,
                   "provided Error Visitor is not exhaustive");
+
+    // Forbid any error handlers that are returning void.
+    // See: https://tracker.ceph.com/issues/69406
     using return_t = std::invoke_result_t<ErrorVisitorT, ErrorT>;
     static_assert(!std::is_same_v<return_t, void>,
                   "error handlers mustn't return void");
+
+    // Any assert_* handler we have:
+    // assert_failure, assert_all and assert_all_func_t
+    // are expected to return void since we actually abort in them.
+    // This is why we need a way to diffreciate between them and between
+    // non-aborting error handlers (e.g handle) - for that we use the dedicated
+    // label of: no_touch_error_marker. Otherwise we would fail the above
+    // static assertion.
     if constexpr (std::is_same_v<return_t, no_touch_error_marker>) {
-      return;
+      [[maybe_unused]] auto &&ep = std::move(result).get_exception();
+      std::ignore = std::invoke(std::forward<ErrorVisitorT>(errfunc),
+                                ErrorT::error_t::from_exception_ptr(std::move(ep)));
     } else {
       // In C++ throwing an exception isn't the sole way to signal
       // error with it. This approach nicely fits cold, infrequent cases
@@ -969,8 +986,8 @@ public:
       static_assert(contains_once_v<std::decay_t<ErrorT>>,
                     "discarding disallowed ErrorT");
       try {
-        std::rethrow_exception(e.ep);
-      } catch(const typename ErrorT::error_type_t& err) {
+        std::rethrow_exception(e.exception_ptr());
+      } catch(const typename std::decay_t<ErrorT>::error_type_t& err) {
         f(err);
       }
       ceph_abort();