Skip to content

Commit d50b94f

Browse files
cyx1231stcyx1231st
authored
Merge pull request ceph#55735 from xxhdx1985126/wip-crimson-errorator-void-handler
crimson/common/errorator: disallow void-returning error handlers Reviewed-by: Samuel Just <[email protected]> Reviewed-by: Radoslaw Zarzynski <[email protected]> Reviewed-by: Yingxin Cheng <[email protected]> Reviewed-by: Chunmei Liu <[email protected]>
2 parents a8099a6 + 0043fa9 commit d50b94f

30 files changed

+223
-185
lines changed

‎src/crimson/common/errorator.h‎

Lines changed: 123 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -156,12 +156,17 @@ class error_t {
156156
}
157157
};
158158

159+
struct no_touch_error_marker {};
160+
159161
// unthrowable_wrapper ensures compilation failure when somebody
160162
// would like to `throw make_error<...>)()` instead of returning.
161163
// returning allows for the compile-time verification of future's
162164
// AllowedErrorsV and also avoid the burden of throwing.
163165
template <class ErrorT, ErrorT ErrorV>
164166
struct unthrowable_wrapper : error_t<unthrowable_wrapper<ErrorT, ErrorV>> {
167+
168+
using error_type_t = ErrorT;
169+
165170
unthrowable_wrapper(const unthrowable_wrapper&) = delete;
166171
[[nodiscard]] static const auto& make() {
167172
static constexpr unthrowable_wrapper instance{};
@@ -204,19 +209,27 @@ struct unthrowable_wrapper : error_t<unthrowable_wrapper<ErrorT, ErrorV>> {
204209

205210
class assert_failure {
206211
const char* const msg = nullptr;
212+
std::function<void()> pre_assert;
207213
public:
208214
template <std::size_t N>
209215
assert_failure(const char (&msg)[N])
210216
: msg(msg) {
211217
}
212218
assert_failure() = default;
219+
template <typename Func>
220+
assert_failure(Func&& f)
221+
: pre_assert(std::forward<Func>(f)) {}
213222

214-
void operator()(const unthrowable_wrapper&) {
223+
no_touch_error_marker operator()(const unthrowable_wrapper&) {
224+
if (pre_assert) {
225+
pre_assert();
226+
}
215227
if (msg) {
216228
ceph_abort(msg);
217229
} else {
218230
ceph_abort();
219231
}
232+
return no_touch_error_marker{};
220233
}
221234
};
222235

@@ -255,6 +268,9 @@ std::exception_ptr unthrowable_wrapper<ErrorT, ErrorV>::carrier_instance = \
255268

256269
template <class ErrorT>
257270
struct stateful_error_t : error_t<stateful_error_t<ErrorT>> {
271+
272+
using error_type_t = ErrorT;
273+
258274
template <class... Args>
259275
explicit stateful_error_t(Args&&... args)
260276
: ep(std::make_exception_ptr<ErrorT>(std::forward<Args>(args)...)) {
@@ -281,6 +297,36 @@ struct stateful_error_t : error_t<stateful_error_t<ErrorT>> {
281297
};
282298
}
283299

300+
class assert_failure {
301+
const char* const msg = nullptr;
302+
std::function<void(const ErrorT&)> pre_assert;
303+
public:
304+
template <std::size_t N>
305+
assert_failure(const char (&msg)[N])
306+
: msg(msg) {
307+
}
308+
assert_failure() = default;
309+
template <typename Func>
310+
assert_failure(Func&& f)
311+
: pre_assert(std::forward<Func>(f)) {}
312+
313+
no_touch_error_marker operator()(stateful_error_t<ErrorT>&& e) {
314+
if (pre_assert) {
315+
try {
316+
std::rethrow_exception(e.ep);
317+
} catch (const ErrorT& err) {
318+
pre_assert(err);
319+
}
320+
}
321+
if (msg) {
322+
ceph_abort(msg);
323+
} else {
324+
ceph_abort();
325+
}
326+
return no_touch_error_marker{};
327+
}
328+
};
329+
284330
private:
285331
std::exception_ptr ep;
286332

@@ -320,44 +366,43 @@ class maybe_handle_error_t {
320366
void handle() {
321367
static_assert(std::is_invocable<ErrorVisitorT, ErrorT>::value,
322368
"provided Error Visitor is not exhaustive");
323-
// In C++ throwing an exception isn't the sole way to signal
324-
// error with it. This approach nicely fits cold, infrequent cases
325-
// but when applied to a hot one, it will likely hurt performance.
326-
//
327-
// Alternative approach is to create `std::exception_ptr` on our
328-
// own and place it in the future via `make_exception_future()`.
329-
// When it comes to handling, the pointer can be interrogated for
330-
// pointee's type with `__cxa_exception_type()` instead of costly
331-
// re-throwing (via `std::rethrow_exception()`) and matching with
332-
// `catch`. The limitation here is lack of support for hierarchies
333-
// of exceptions. The code below checks for exact match only while
334-
// `catch` would allow to match against a base class as well.
335-
// However, this shouldn't be a big issue for `errorator` as Error
336-
// Visitors are already checked for exhaustiveness at compile-time.
337-
//
338-
// NOTE: `__cxa_exception_type()` is an extension of the language.
339-
// It should be available both in GCC and Clang but a fallback
340-
// (based on `std::rethrow_exception()` and `catch`) can be made
341-
// to handle other platforms if necessary.
342-
if (type_info == ErrorT::error_t::get_exception_ptr_type_info()) {
343-
// set `state::invalid` in internals of `seastar::future` to not
344-
// call `report_failed_future()` during `operator=()`.
345-
[[maybe_unused]] auto&& ep = std::move(result).get_exception();
346-
347-
using return_t = std::invoke_result_t<ErrorVisitorT, ErrorT>;
348-
if constexpr (std::is_assignable_v<decltype(result), return_t>) {
349-
result = std::invoke(std::forward<ErrorVisitorT>(errfunc),
350-
ErrorT::error_t::from_exception_ptr(std::move(ep)));
351-
} else if constexpr (std::is_same_v<return_t, void>) {
352-
// void denotes explicit discarding
353-
// execute for the sake a side effects. Typically this boils down
354-
// to throwing an exception by the handler.
355-
std::invoke(std::forward<ErrorVisitorT>(errfunc),
356-
ErrorT::error_t::from_exception_ptr(std::move(ep)));
357-
} else {
358-
result = FuturatorT::invoke(
359-
std::forward<ErrorVisitorT>(errfunc),
360-
ErrorT::error_t::from_exception_ptr(std::move(ep)));
369+
using return_t = std::invoke_result_t<ErrorVisitorT, ErrorT>;
370+
static_assert(!std::is_same_v<return_t, void>,
371+
"error handlers mustn't return void");
372+
if constexpr (std::is_same_v<return_t, no_touch_error_marker>) {
373+
return;
374+
} else {
375+
// In C++ throwing an exception isn't the sole way to signal
376+
// error with it. This approach nicely fits cold, infrequent cases
377+
// but when applied to a hot one, it will likely hurt performance.
378+
//
379+
// Alternative approach is to create `std::exception_ptr` on our
380+
// own and place it in the future via `make_exception_future()`.
381+
// When it comes to handling, the pointer can be interrogated for
382+
// pointee's type with `__cxa_exception_type()` instead of costly
383+
// re-throwing (via `std::rethrow_exception()`) and matching with
384+
// `catch`. The limitation here is lack of support for hierarchies
385+
// of exceptions. The code below checks for exact match only while
386+
// `catch` would allow to match against a base class as well.
387+
// However, this shouldn't be a big issue for `errorator` as Error
388+
// Visitors are already checked for exhaustiveness at compile-time.
389+
//
390+
// NOTE: `__cxa_exception_type()` is an extension of the language.
391+
// It should be available both in GCC and Clang but a fallback
392+
// (based on `std::rethrow_exception()` and `catch`) can be made
393+
// to handle other platforms if necessary.
394+
if (type_info == ErrorT::error_t::get_exception_ptr_type_info()) {
395+
// set `state::invalid` in internals of `seastar::future` to not
396+
// call `report_failed_future()` during `operator=()`.
397+
[[maybe_unused]] auto &&ep = std::move(result).get_exception();
398+
if constexpr (std::is_assignable_v<decltype(result), return_t>) {
399+
result = std::invoke(std::forward<ErrorVisitorT>(errfunc),
400+
ErrorT::error_t::from_exception_ptr(std::move(ep)));
401+
} else {
402+
result = FuturatorT::invoke(
403+
std::forward<ErrorVisitorT>(errfunc),
404+
ErrorT::error_t::from_exception_ptr(std::move(ep)));
405+
}
361406
}
362407
}
363408
}
@@ -389,6 +434,7 @@ static constexpr auto composer(FuncHead&& head, FuncTail&&... tail) {
389434
std::is_invocable_v<FuncHead, decltype(args)...> ||
390435
(sizeof...(FuncTail) > 0),
391436
"composition is not exhaustive");
437+
return no_touch_error_marker{};
392438
}
393439
};
394440
}
@@ -887,14 +933,6 @@ struct errorator {
887933
}
888934
};
889935

890-
struct discard_all {
891-
template <class ErrorT, EnableIf<ErrorT>...>
892-
void operator()(ErrorT&&) {
893-
static_assert(contains_once_v<std::decay_t<ErrorT>>,
894-
"discarding disallowed ErrorT");
895-
}
896-
};
897-
898936
template <typename T>
899937
static future<T> make_errorator_future(seastar::future<T>&& fut) {
900938
return std::move(fut);
@@ -911,17 +949,46 @@ struct errorator {
911949
assert_all() = default;
912950

913951
template <class ErrorT, EnableIf<ErrorT>...>
914-
void operator()(ErrorT&&) {
952+
no_touch_error_marker operator()(ErrorT&&) {
915953
static_assert(contains_once_v<std::decay_t<ErrorT>>,
916954
"discarding disallowed ErrorT");
917955
if (msg) {
918956
ceph_abort_msg(msg);
919957
} else {
920958
ceph_abort();
921959
}
960+
return no_touch_error_marker{};
922961
}
923962
};
924963

964+
template <typename Func>
965+
class assert_all_func_t {
966+
public:
967+
assert_all_func_t(Func &&f)
968+
: f(std::forward<Func>(f)) {}
969+
970+
template <class ErrorT, EnableIf<ErrorT>...>
971+
no_touch_error_marker operator()(ErrorT&& e) {
972+
static_assert(contains_once_v<std::decay_t<ErrorT>>,
973+
"discarding disallowed ErrorT");
974+
try {
975+
std::rethrow_exception(e.ep);
976+
} catch(const typename ErrorT::error_type_t& err) {
977+
f(err);
978+
}
979+
ceph_abort();
980+
return no_touch_error_marker{};
981+
}
982+
983+
private:
984+
Func f;
985+
};
986+
987+
template <typename Func>
988+
static auto assert_all_func(Func &&f) {
989+
return assert_all_func_t<Func>{std::forward<Func>(f)};
990+
}
991+
925992
template <class ErrorFunc>
926993
static decltype(auto) all_same_way(ErrorFunc&& error_func) {
927994
return all_same_way_t<ErrorFunc>{std::forward<ErrorFunc>(error_func)};
@@ -1240,28 +1307,29 @@ namespace ct_error {
12401307
}
12411308
};
12421309

1243-
struct discard_all {
1244-
template <class ErrorT>
1245-
void operator()(ErrorT&&) {
1246-
}
1247-
};
1248-
12491310
class assert_all {
12501311
const char* const msg = nullptr;
1312+
std::function<void()> pre_assert;
12511313
public:
12521314
template <std::size_t N>
12531315
assert_all(const char (&msg)[N])
12541316
: msg(msg) {
12551317
}
12561318
assert_all() = default;
1319+
assert_all(std::function<void()> &&f)
1320+
: pre_assert(std::move(f)) {}
12571321

12581322
template <class ErrorT>
1259-
void operator()(ErrorT&&) {
1323+
no_touch_error_marker operator()(ErrorT&&) {
1324+
if (pre_assert) {
1325+
pre_assert();
1326+
}
12601327
if (msg) {
12611328
ceph_abort(msg);
12621329
} else {
12631330
ceph_abort();
12641331
}
1332+
return no_touch_error_marker{};
12651333
}
12661334
};
12671335

‎src/crimson/common/interruptible_future.h‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -821,13 +821,17 @@ class [[nodiscard]] interruptible_future_detail<
821821
}, [func=std::move(errfunc),
822822
interrupt_condition=interrupt_cond<InterruptCond>.interrupt_cond]
823823
(auto&& err) mutable -> decltype(auto) {
824-
constexpr bool return_void = std::is_void_v<
824+
static_assert(!std::is_void_v<
825825
std::invoke_result_t<ErrorVisitorT,
826-
std::decay_t<decltype(err)>>>;
826+
std::decay_t<decltype(err)>>>);
827+
constexpr bool is_assert = std::is_same_v<
828+
std::decay_t<std::invoke_result_t<ErrorVisitorT,
829+
std::decay_t<decltype(err)>>>,
830+
::crimson::no_touch_error_marker>;
827831
constexpr bool return_err = ::crimson::is_error_v<
828832
std::decay_t<std::invoke_result_t<ErrorVisitorT,
829833
std::decay_t<decltype(err)>>>>;
830-
if constexpr (return_err || return_void) {
834+
if constexpr (return_err || is_assert) {
831835
return non_futurized_call_with_interruption(
832836
interrupt_condition,
833837
std::move(func),
@@ -857,13 +861,17 @@ class [[nodiscard]] interruptible_future_detail<
857861
}, [func=std::move(errfunc),
858862
interrupt_condition=interrupt_cond<InterruptCond>.interrupt_cond]
859863
(auto&& err) mutable -> decltype(auto) {
860-
constexpr bool return_void = std::is_void_v<
864+
static_assert(!std::is_void_v<
861865
std::invoke_result_t<ErrorVisitorT,
862-
std::decay_t<decltype(err)>>>;
866+
std::decay_t<decltype(err)>>>);
867+
constexpr bool is_assert = std::is_same_v<
868+
std::decay_t<std::invoke_result_t<ErrorVisitorT,
869+
std::decay_t<decltype(err)>>>,
870+
::crimson::no_touch_error_marker>;
863871
constexpr bool return_err = ::crimson::is_error_v<
864872
std::decay_t<std::invoke_result_t<ErrorVisitorT,
865873
std::decay_t<decltype(err)>>>>;
866-
if constexpr (return_err || return_void) {
874+
if constexpr (return_err || is_assert) {
867875
return non_futurized_call_with_interruption(
868876
interrupt_condition,
869877
std::move(func),
@@ -985,13 +993,17 @@ class [[nodiscard]] interruptible_future_detail<
985993
[errfunc=std::move(errfunc),
986994
interrupt_condition=interrupt_cond<InterruptCond>.interrupt_cond]
987995
(auto&& err) mutable -> decltype(auto) {
988-
constexpr bool return_void = std::is_void_v<
996+
static_assert(!std::is_void_v<
989997
std::invoke_result_t<ErrorFunc,
990-
std::decay_t<decltype(err)>>>;
998+
std::decay_t<decltype(err)>>>);
999+
constexpr bool is_assert = std::is_same_v<
1000+
std::decay_t<std::invoke_result_t<ErrorFunc,
1001+
std::decay_t<decltype(err)>>>,
1002+
::crimson::no_touch_error_marker>;
9911003
constexpr bool return_err = ::crimson::is_error_v<
9921004
std::decay_t<std::invoke_result_t<ErrorFunc,
9931005
std::decay_t<decltype(err)>>>>;
994-
if constexpr (return_err || return_void) {
1006+
if constexpr (return_err || is_assert) {
9951007
return non_futurized_call_with_interruption(
9961008
interrupt_condition,
9971009
std::move(errfunc),

‎src/crimson/os/cyanstore/cyan_store.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,10 @@ class CyanStore final : public FuturizedStore {
185185
return shard_stores.invoke_on_all(
186186
[](auto &local_store) {
187187
return local_store.mount().handle_error(
188-
crimson::stateful_ec::handle([](const auto& ec) {
188+
crimson::stateful_ec::assert_failure([](const auto& ec) {
189189
crimson::get_logger(ceph_subsys_cyanstore).error(
190190
"error mounting cyanstore: ({}) {}",
191191
ec.value(), ec.message());
192-
std::exit(EXIT_FAILURE);
193192
}));
194193
});
195194
}

‎src/crimson/os/seastore/async_cleaner.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,9 +299,7 @@ class ExtentCallbackInterface {
299299
return do_with_transaction_intr<Func, true>(
300300
Transaction::src_t::READ, name, std::forward<Func>(f)
301301
).handle_error(
302-
crimson::ct_error::eagain::handle([] {
303-
ceph_assert(0 == "eagain impossible");
304-
}),
302+
crimson::ct_error::eagain::assert_failure{"unexpected eagain"},
305303
crimson::ct_error::pass_further_all{}
306304
);
307305
}

‎src/crimson/os/seastore/journal/record_submitter.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ RecordSubmitter::roll_segment()
255255
has_io_error = true;
256256
wait_available_promise->set_value();
257257
wait_available_promise.reset();
258+
return seastar::now();
258259
})
259260
).handle_exception([FNAME, this](auto e) {
260261
ERROR("{} got exception {}, available", get_name(), e);
@@ -522,6 +523,7 @@ void RecordSubmitter::flush_current_batch()
522523
ERROR("{} {} records, {}, got error {}",
523524
get_name(), num, sizes, e);
524525
finish_submit_batch(p_batch, std::nullopt);
526+
return seastar::now();
525527
})
526528
).handle_exception([this, p_batch, FNAME, num, sizes=sizes](auto e) {
527529
ERROR("{} {} records, {}, got exception {}",

0 commit comments

Comments
 (0)