kvrocks
kvrocks copied to clipboard
apache
Add `StatusOr` for error handling in modern C++ style
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.
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.
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
StatusOrand 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
Statuswithout major performance issues, so I thinkStatusis 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.
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.
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.
Hi everyone, feel free to review and ask questions.
If there is no further discussion, I will merge it. :rocket:
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.
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
autoto deduce the output type because we have to construct it before calling the function. - as described in here,
StatusOrcan 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.
@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
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
@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
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.
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: https://github.com/apache/doris/discussions/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.
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: https://github.com/apache/doris/discussions/8406 , and some doesn't like it because of performance lost, like https://github.com/scylladb/seastar/issues/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.
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.
