Add StatusOr for error handling in modern C++ style

[PragmaTwice]

In kvrocks we use a style like Status process(input..., T* output) to handle errors. It works well, but in C++ we can do better via constructing type-safe union types.

In this PR, we propose a new class template StatusOr<T>, which contains EITHER a value typed T OR an error status. Here is a simple example:

StatusOr<std::string> hello(std::string x) {
  if (x.size() > 10) {
    return {Status::NotOK, "string too long"}; // returns an error
  }

  return x + " hello"; // returns a value (no error occurred)
};

StatusOr<std::string> hi(std::string x) {
  if (x.size() < 5) {
    return {Status::NotOK, "string too short"};
  }

  auto res = hello(x); // call `hello` which returns `StatusOr`
  if (!res) return res; // forward error status

  return "hi " + *res; // use it via deref
};

assert(*hi("twice") == "hi twice hello");

auto s = hi("x");
assert(!s && s.Msg() == "string too short"); // s has no value now

auto l = hi("xxxxxxxxxxx");
assert(!l && l.Msg() == "string too long");

We maximize the use of move semantics in C++ to eliminate redundant copies and optimize the storage, so that developers do not need to worry too much about its performance when using it.

[caipengbo]

This approach may be a more modern C++ approach, but I don't think it's very friendly for a rookie like me.
It may take me some time to get familiar with how to use StatusOr and what the implementation means, which may not be very friendly for a beginner like me to contribute code.

RocksDB as the storage engine, also uses the Status without major performance issues, so I think Status is better.

[PragmaTwice]

This approach may be a more modern C++ approach, but I don't think it's very friendly for a rookie like me. It may take me some time to get familiar with how to use StatusOr and what the implementation means, which may not be very friendly for a beginner like me to contribute code.

RocksDB as the storage engine, also uses the Status without major performance issues, so I think Status is better.

First of all, I do not think contributors need to understand the implementation of StatusOr, just as they do not need to understand the implementation of std::optional (c++17), std::varaint (c++17), std::expected (c++23), std::any or any other containers (or called class templates) in STL (or called standard library). The interface of StatusOr is straightforward enough for all usage without learning its implementation. (You can imagine it as an optional value like std::optional<T>, except there is an error status while there is no value in the object. Check tagged unions in wiki for further understanding.)

Secondly, the idea of StatusOr comes from abseil, which is the fundamental library in Google used for almost all other Google technologies and products. So I do not think there is any evident performance downgrade to use it (even there is some implementation difference between this and the abseil one). And obviously RocksDB maintainers do not practice modern C++ (or even C++) well so I think we do not need to follow them in every part.

Finally, all use of Status will not be replaced immediately. We can continue to use the traditional pattern in all part of the repo. I will apply this novel class template to construct new framework for command parsing and other utilities.

[PragmaTwice]

In fact, I think StatusOr is not only more expressive, but also performs better than Status in some cases.

This is because Status is a large structure (std::string is usually larger than two words because SSO (small string optimization) is generally used in the implementation of std::string, for example, the implementation of std::string in libstdc++ is 32 bytes under 64 bits), so just Status is 40 bytes long, not including pointers for output. And Status inevitably constructs a std::string for any execution path.

But StatusOr instance like StatusOr<int> is only 16 bytes long and does not initialize std::string when no error occurs, and it initializes the resulting integer directly in-place inside these 16 bytes.

[git-hulk]

To be honest, this change looks a bit complex at first glance.
But after taking a look at how the Status/StatusOr works and uses,
I think it can simplify a lot on how we add a new status code
and return a status with value.

For myself, I would happy to see that we can use the modern way
to improve our codebase, even it needs some time to learn for guys
came from C or legacy C++ code style like me.

[ShooterIT]

what would happen if calling f(11).GetValue()?

[PragmaTwice]

Hi @ShooterIT, if you look at the definition of StatusOr<T>::GetValue:

  value_type& GetValue() & {
    CHECK(*this);
    return getValue();
  }

  value_type&& GetValue() && {
    CHECK(*this);
    return std::move(getValue());
  }

  const value_type& GetValue() const& {
    CHECK(*this);
    return getValue();
  }

You will find there is three different overload of this method via changing the qualifier of *this.

Hence, in f(x).GetValue(), obviously f(x) is a prvalue since a temporary object is returned, and then the signature value_type&& GetValue() && is decided to be called after the overload resolution, and thus you get an xvalue (which is a special kind of rvalue) referenced to the underlying value of the returned StatusOr.

So we should take care of the lifetime in this case, the rvalue will become invalid after the computation of the current statement. We can extend it via a call to a move ctor (T(T&&)). This is not a defect of this type, but it is an idiom of C++, we should always care about lifetime of references to avoid dangling references in all standards of C++ (98/03/11/14/17/20 ...).

[PragmaTwice]

BTW, the classic usage of StatusOr<T> we expected is:

// 1
StatusOr<T> someProcess(...); // as a return type

// 2
T val;
...
return val; // return val typed T to StatusOr<T>

// 3
return {Status::someErrCode, "msg..."}; // return an error status

// 4
auto res = someProcess(...); // get result typed StatusOr<T>
if(!res) return res; // forward it if it is an error status

doSomething(*res); // process the value inside the StatusOr

Use StatusOr as an rvalue is totally (well) supported, but everyone should be aware of what they are doing, otherwise just follow the classic usage is fine.

[ShooterIT]

i want to describe 11 is greater than 10, GetValue() will return error?

[PragmaTwice]

GetValue() will trigger a fatal error.
It is just like you call f as Status f(int *output), but do not check the returned Status and directly dereference the output. It is not allowed, you must check status before dereference.

[tisonkun]

I think that guarded by CHECK(*this), it can cause a fatal error.

I wonder if we can generate a compile time error instead of runtime fatal in this case.

[tisonkun]

Confirm its failure as:

WARNING: Logging before InitGoogleLogging() is written to STDERR
F20220812 15:30:19.518088 1124890 status.h:202] Check failed: *this 
*** Check failure stack trace: ***

[PragmaTwice]

Hi @tisonkun, thanks for your review.

I wonder if we can generate a compile time error instead of runtime fatal in this case.

Good idea. We consider an example for this:

auto res = someProcess(...); // get result typed StatusOr<T>
// we cannot deref `res` here

if(!res) { 
  processError(res.GetCode(), res.Msg()); /* or */ return res; // we cannot deref `res` here
}

doSomething(*res); // we can deref it

In this example, only the doSomething part can actually dereference the res.
We cannot dereference it both before if(!res) and inside if(!res).
So only in the execution path with the condition res.IsOk() == true being satisfied, we can dereference res,

It is hardly to express in compile time, since it is related to the control flow.
Like a raw/smart pointer, we should check it does point to a value (not null) before dereference it.

I have implemented a checker in Clang Static Analyzer (included in clang-tidy) for OneFlow that checks whether a type like StatusOr is dereferenced after checking IsOk() using symbolic execution, and I think it can be done in this way in the future.

[tisonkun]

checks whether a type like StatusOr is dereferenced after checking IsOk() using symbolic execution

Sounds good to me.

[PragmaTwice]

Hi everyone, feel free to review and ask questions.

If there is no further discussion, I will merge it. 🚀

[ShooterIT]

Please wait a minute, i still need some time to put my thoughts

[ShooterIT]

To be honest, this is the first time to see this usage. I study your PR and https://abseil.io/docs/cpp/guides/status, i find it truly satisfies my requirement that i want to return status or value if ok when calling function, a bit like multi return value, such as

StatusOr<Foo> result = Calculation();
if (result.ok()) {
  result->DoSomethingCool();
} else {
  LOG(ERROR) << result.status();
}

before you PR, our usage is like the following:

MyType myvalue;
Status s = Func(&myvalue);
if (s.ok()) {
  DoThings(myvalue);
} else {
  ErrorReport();
}

I think you may want to refactor these usages above, right?

i am not a c++ language expert, i don't see this usage frequently, i think there are a few people accept quickly, so i am not sure we should merge this. Maybe i am lazy, and not want to learn new knowledge.

[PragmaTwice]

Hi @ShooterIT, your understanding is generally correct. I'm still holding on to the hope that we'll be able to merge this PR, which I state below on various fronts.

First, the original error handling method has several flaws:

• the pointer passed in to store the result is readable, C++ doesn't have any flag to indicate that a pointer is write-only: this causes semantic confusion, in fact, we don't need the value the pointer is currently pointing to, this value is garbage when the result is not written;
• the pointer passed in to store the result points to an already constructed object, which is usually default-constructed: this prevents us from constructing an object after processing (probably not via the default constructor), which It's a very popular way of doing things in C++;
• we cannot use auto to deduce the output type because we have to construct it before calling the function.
• as described in here, StatusOr can be more efficient than the original pattern in various cases.

Second, StatusOr is designed to be very intuitive. This type has two states, one in which it stores a value representing the result, and one in which it stores an error (including an error code and error message). I always feel that StatusOr will be easier to use than the current form, and we can more intuitively express what we want to do.

Lastly, I have no current plans to replace any part of the current project that use Status, and I've even modified Status in this PR to make better use of move semantics in certain situation. Anyone can continue to use the previous form for error handling, it's just that I'll use it to make it easier to build some command parsing related functions.

[wy-ei]

@PragmaTwice

If using exception to report error instead of status, we don't need Status and this StatusOr at all.

What is the better way to report error in C++? error code or exception? Here are same posts to discuss this question:

• https://isocpp.org/wiki/faq/exceptions
• https://stackoverflow.com/questions/253314/conventions-for-exceptions-or-error-codes

[PragmaTwice]

@PragmaTwice

If using exception to report error instead of status, we don't need Status and this StatusOr at all.

What is the better way to report error in C++? error code or exception? Here are same posts to discuss this question:

• https://isocpp.org/wiki/faq/exceptions
• https://stackoverflow.com/questions/253314/conventions-for-exceptions-or-error-codes

Hi @wy-ei , a large part of C++ projects do not use exception as the mainly error handling method (but use it while some fatal or very accidental error occurred) since exception is relatively expensive in bad path (or say, failure path, maybe x10 - x20 slower than conditional jumping, but zero-cost on happy path)[1-2], like all Google products, LLVM projects, etc. So I don't think it's a good idea to use exceptions on some execution path that is not a very low probability error or is user controllable.

Another defect is that C++ does not have checked exception, this makes exception flows not easy to maintain. In addition, it is not easy to ensure the exception safety of the program, which requires the programmer to have a deep background knowledge of C++. So I don't have a strong desire to use exceptions. Some PL like Rust make heavy use of sum types to express potentially error-prone results, and uses a functional approach to provide higher-order composition, which I think is easier to use than exceptions.

[1] https://pspdfkit.com/blog/2020/performance-overhead-of-exceptions-in-cpp
[2] https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2544r0.html

[wy-ei]

@PragmaTwice

Throwing exception is more than 100 times slower than return error code, If the probability of throwing exception is less than 1/100, the cost of throwing exception can be ignored.

I run the benchmark in [1] on my machine, here is the result:

Run on (8 X 24.1247 MHz CPU s)
CPU Caches:
  L1 Data 64 KiB
  L1 Instruction 128 KiB
  L2 Unified 4096 KiB (x8)
Load Average: 4.60, 3.61, 3.14
----------------------------------------------------------------------
Benchmark                            Time             CPU   Iterations
----------------------------------------------------------------------
BM_exitWithBasicException          356 ns          356 ns      1949354
BM_exitWithMessageException        423 ns          412 ns      1812467
BM_exitWithReturn                 3.09 ns         3.09 ns    218084841
BM_exitWithErrorCode              3.04 ns         3.04 ns    233471081

In this benchmark, the exception was thrown every two function calls, but in real situation, exception unlikely be thrown this often.

I change the benchmark code to make the exception be thrown every 200 function calls. (change the variable randomRange to 200)，and run benchmark again, here is the result:

Run on (8 X 24.1216 MHz CPU s)
CPU Caches:
  L1 Data 64 KiB
  L1 Instruction 128 KiB
  L2 Unified 4096 KiB (x8)
Load Average: 3.45, 2.54, 3.10
----------------------------------------------------------------------
Benchmark                            Time             CPU   Iterations
----------------------------------------------------------------------
BM_exitWithBasicException         6.78 ns         6.78 ns     75871712
BM_exitWithMessageException       7.03 ns         7.03 ns     99061744
BM_exitWithReturn                 2.98 ns         2.98 ns    235143942
BM_exitWithErrorCode              2.98 ns         2.98 ns    234956097

Since throwing exception is a small part of the code, in real situation, I think the cost of exception is not a big problem.

So I don't think it's a good idea to use exceptions on some execution path that is not a very low probability error or is user controllable.

If user input cause error, then error may occur with high probability, use exception would be inefficient. In this situation, some pre check code can be added to return error fast, and comparing with disk and network read and write, the cost of exception is not the main part.

Another defect is that C++ does not have checked exception, this makes exception flows not easy to maintain. In addition, it is not easy to ensure the exception safety of the program, which requires the programmer to have a deep background knowledge of C++.

C++ do not force user catch the exception, but if you miss an exception the process will crash. We can ignore an error status, and something weird happened, but we didn't known the reason. If we keep using RAII, the exception safety can almost be ensure.

I mentioned exception here because I was think is there another way to deal with error instead Status. I think both exception and status are ok. But using exception in kvrocks will break the consistency of the function design (return Status and pass result as a pointer in last argument), I think this will be a big change, and I except this won't happed in the near future.

I was off topic for a long time, Back to Status and StatusOr.

In kvrocks, Status class is a large object which include a std::string in it. The Status class implemented in leveldb only take 8 bytes (only one member which is const char *state_). If status is ok, state_ is nullptr, if status is not ok, the state_ store the error code and error message. If we change the Status implementation in kvrocks to the leveldb way, the size of Status is not a problem.

Another problem of Status for me is it occupy the place of the return value, we must pass a pointer to take the return value. Golang solve this problem by returning multi value. A sample and easily understand way is return a tuple or another Class will contain return value T and status.

For StatusOr I have one question. StatusOr use a char[] to store T or Status, I don't known why, why don't use union?

C++23 std::expected [2] is used to contains expected value or an error, does this a more generic way, Can we implement same thing like this. We can combine Status and the expected class do the things StatusOr does.

[1] https://pspdfkit.com/blog/2020/performance-overhead-of-exceptions-in-cpp
[2] https://en.cppreference.com/w/cpp/header/expected

[PragmaTwice]

C++23 std::expected [2] is used to contains expected value or an error, does this a more generic way, Can we implement same thing like this. We can combine Status and the expected class do the things StatusOr does.

Hi @wy-ei , StatusOr<T> has same purpose and similar implementation with std::expected<T, E>. Actually I has implemented something equal to std::expected for oneflow, and you can find its source code here, oneflow::maybe::Maybe<T, E>, oneflow::maybe::Variant<T...> and oneflow::maybe::Optional<T> is the basic error handling method in oneflow.

You can check Oneflow-Inc/oneflow/pull/6820 for more details.

But obviously, the implementation of std::expected is only more complicated and difficult to understand, while the implementation of StatusOr is relatively simple.

For StatusOr I have one question. StatusOr use a char[] to store T or Status, I don't known why, why don't use union?

The same problem is faced with the union implementation, you also need to evaluate some conditions before calling the ctor/dtor, so I don't see a difference. And you also need to manual call dtor and carefully handle lifetime in union for non-POD types.

The Status class implemented in leveldb only take 8 bytes (only one member which is const char *state_). If status is ok, state_ is nullptr, if status is not ok, the state_ store the error code and error message. If we change the Status implementation in kvrocks to the leveldb way, the size of Status is not a problem.

I think it's only a little different from std::unique_ptr<Status> and performs worse than StatusOr, since error code is on the stack in StatusOr.

Another problem of Status for me is it occupy the place of the return value, we must pass a pointer to take the return value. Golang solve this problem by returning multi value. A sample and easily understand way is return a tuple or another Class will contain return value T and status.

This is actually a huge flaw in golang: golang has no sum type. So in golang, both the two values (the actual value and an error) should be returned instead of expressing an "either .. or .." relationship. Of course, interface and generics can achieve this, but it is obviously far more troublesome than sum type.

[git-hulk]

I wonder why kvrocks' Status uses std::string, it would be large, likely 24B on stack

To be honest, we didn't think carefully about this case before, because AFAIK the bottleneck
of Kvrocks is commonly at RocksDB or disk io latency. Of course, I think it will be good to optimize as much as possible if it can keep the code base simple at the same time.

Secondly, sometimes when we want to find the root cause of the problem, status make it hard to debug. apache doris thinks using exception is a better method: apache/doris#8406

I guess the reason why no one mentioned this issue: call the chain of Kvrocks is simple engouh: command->data structure->rocksdb, so we can tell the root cause even without exception stacks. Correct me if others don't agree with me.

[mapleFU]

I wonder why kvrocks' Status uses std::string, it would be large, likely 24B on stack ( for example: https://github.com/llvm-mirror/libcxx/blob/master/include/string#L765-L784 )

When I gothrough the code of project, I found that status code is widely used, introducing exception might be ok or not (some says it can help trace the error stack: apache/doris#8406 , and some doesn't like it because of performance lost, like scylladb/seastar#73 . But introducing exception will change the code style greatly, so I don't thinks it's a good idea to change to exception.

Using StatusOr can change the error handling style without changing lots of codes, and it doesn't need changing other code using Status, so I think is ok to introducing it.

[git-hulk]

Yes, I also think StatusOr is more intuitive than before

[jackwener]

Agree with use StatusOr

• No extensive code changes required
• It's easy to use and more intuitive
• Morden C++ style

StatusOr is also used by folly which is used wildly.

[tisonkun]

Generally looks good.

One comment is inline at https://github.com/apache/incubator-kvrocks/pull/768/files#r944171514.

Another comment is that where we use StatusOr outside status.h or you plan to do so? Otherwise, we trade off new code snippets for none or little usage which seems redundant.

UPDATE: I see this comment now.

We maximize the use of move semantics in C++ to eliminate redundant copies and optimize the storage, so that developers do not need to worry too much about its performance when using it.

[tisonkun]

Merging...