Employ consistent error handling policy and make fatal error conditions fail fast

[dennisklein]

Triggered by a discussion in the Alice WP3 mailing list about error handling, I propose to employ the following error handling "policy" (more in the sense of a guideline and less in the sense of a strict ruleset, simply to achieve more consistency over randomness) for FairMQ:

1. State pre and postconditions via asserts / comments, violations are code bugs and need fixing, not exceptions
   • Apply throughout the current FairMQ codebase and collect and document practical examples
   • Decide whether to tie asserts to NDEBUG or a separate switch (gsl's runtime asserts are not disabable at all even for release builds)
2. Prefer noexcept and strongly typed error codes via return channel - in the future: replace error codes with static exceptions
   • Study cases where a noexcept interface internally calls noexcept(false) interfaces and decide if the "Prefer noexcept" can hold as a guideline.
   • Study cases where we already use C-style int error codes that do not distinguish error codes from return value by type and decide whether it is worth changing them.
   • Decide for one of <system_error>, std::experimental::expected, Boost.Outcome or similar
   • Practical examples
3. Throw only unrecoverable (that lead to a terminate) error condition from free and member functions (should basically boil down to std::bad_alloc and std::runtime_error("not implemented")).
   • Understand the consequences of a full stack unwind + automatic terminate vs early std::terminate, especially in combination with noexcept interfaces, with regard to debuggability
4. Throw recoverable error conditions from ctors if class invariant cannot be established (to avoid 2-phase construction). Prefer to throw own exception types (possibly inheriting from a STL base exception type).
   • Practical examples

The above work-in-progress guidelines try to optimize for

1. debuggability/maintainability and
2. runtime performance (should really mainly be achieved via rule 1 and only on second order via rule 2).

Once the above points are finished:

• Introduce new section in FairMQ docs
• Report the results back to Alice to possibly be taken into account when developing further guidelines for Alice O2 projects (@ihrivnac suggested https://github.com/AliceO2Group/CodingGuidelines/master/coding_guidelines.html#Exceptions, @ktf suggested https://github.com/AliceO2Group/AliceO2/blob/dev/Framework/Core/README.md#error-handling)

Cc: @davidrohr, @ihrivnac, @ktf just FYI, this will take a bit to finish, but I guess we are not in a huge hurry here and it should not really block any activities in Alice O2 projects.

References:

• http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-errors
• http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0709r4.pdf
• https://google.github.io/styleguide/cppguide.html#Exceptions
• https://www.youtube.com/watch?v=hNaLf8lYLDo
• https://www.youtube.com/watch?v=koTf7u0v41o

[mattkretz]

should assertions depend on a macro (1.b)?

The following considerations are relevant:

1. If the assertion function is implemented using an unlikely branch (if (!cond) [[unlikely]] abort();), then the CPU should typically predict the non-failure code path to be taken and thus only require independent (i.e. might be hidden completely in ILP) execution resources of the CPU core. Consequently, the assertion might not slow down the code.
2. By introducing the assertion branch, which depends on a certain value range of one or more variables, the compiler can sometimes optimize the non-failure branch better than without the assertion (because it can make assumptions about value ranges). Consequently, code with assertions might even run faster than if the assertions are deleted from the code.
3. For max performance, the assertions can be turned into if (!cond) __builtin_unreachable(); which provides the benefit of the second point without the execution and code size overhead of the assertion itself. However, if, when running the application, the assertion would fail, very strange and hard to debug runtime errors occur.

[dennisklein]

should assertions depend on a macro (1.b)?

The following considerations are relevant:

1. If the assertion function is implemented using an unlikely branch (`if (!cond) [[unlikely]] abort();`), then the CPU should typically predict the non-failure code path to be taken and thus only require independent (i.e. might be hidden completely in ILP) execution resources of the CPU core. Consequently, the assertion might not slow down the code.

2. By introducing the assertion branch, which depends on a certain value range of one or more variables, the compiler can sometimes optimize the non-failure branch better than without the assertion (because it can make assumptions about value ranges). Consequently, _code with assertions might even run faster than if the assertions are deleted_ from the code.

3. For max performance, the assertions can be turned into `if (!cond) __builtin_unreachable();` which provides the benefit of the second point without the execution and code size overhead of the assertion itself. However, if, when running the application, the assertion would fail, very strange and hard to debug runtime errors occur.

1 and 2 sound like one always wants them (currently we do Release and RelWithDebugInfo builds with O2, is it enough for mentioned optimizations to be found?), 3 feels like an optional optimization, which we could add as a compile-time decision, e.g.

#include <cstdlib>

namespace fair::mq {
  inline void AssertPre(bool cond) noexcept { if(!cond) { [[unlikely]] @ABORT@; } }
  inline void AssertPost(bool cond) noexcept { if(!cond) { [[unlikely]] @ABORT@; } }
}

At CMake configure time @ABORT@ would be replaced with std::abort() or __builtin_unreachable() depending on user choice. Am I forgetting anything? So, no need for a macro then.

[dennisklein]

Ok, [[unlikely]] is C++20 (we still support C++17-only), so needs a bit more and probably some log message, and to decorate it, one needs macros still in C++17 -.-